Skip to content

Conversation

@bllchmbrs
Copy link
Contributor

What changes were proposed in this pull request?

We didn't enforce analyzed plans in Spark 2.1 when writing out to Kafka.

How was this patch tested?

New unit test.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Bill Chambers added 2 commits April 28, 2017 07:50
@bllchmbrs
Copy link
Contributor Author

cc @brkyvz this should be good to go.

@SparkQA
Copy link

SparkQA commented Apr 28, 2017

Test build #76271 has finished for PR 17804 at commit 05f17c6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 28, 2017

Test build #76272 has finished for PR 17804 at commit 77e25f1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@hvanhovell
Copy link
Contributor

@anabranch can you open this against branch-2.1 instead of master?

@brkyvz
Copy link
Contributor

brkyvz commented Apr 28, 2017

@hvanhovell We need this fix in master as well. It somehow works magically in master, but is broken for sure in 2.1

@bllchmbrs
Copy link
Contributor Author

yeah it's broken in both, only visible in 1. Not sure if that needs to be in 2 PRs.

Copy link
Contributor

@brkyvz brkyvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nits, LGTM. Will merge pending tests + nit addressing


import org.apache.spark.SparkException
import org.apache.spark.sql._
import org.apache.spark.sql.catalyst.analysis.UnresolvedException
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: unnecessary import

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

import org.apache.spark.sql.types.{BinaryType, DataType}

class KafkaSinkSuite extends StreamTest with SharedSQLContext {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: delete line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

.option("kafka.bootstrap.servers", testUtils.brokerAddress)
.option("topic", topic)
.save()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

val topic = newTopic()
testUtils.createTopic(topic)

inputEvents.write
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: wrong indentation. + I would add a comment here that this would used to throw an UnresolvedException

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

s"save mode overwrite not allowed for kafka"))
}

test("batch - enforce analyzed plans SPARK-20496") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: SPARK-20496: batch - enforce analyzed plans

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@brkyvz
Copy link
Contributor

brkyvz commented Apr 28, 2017

LGTM!

@SparkQA
Copy link

SparkQA commented Apr 28, 2017

Test build #76275 has finished for PR 17804 at commit 4ad21fe.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@brkyvz
Copy link
Contributor

brkyvz commented Apr 28, 2017

merging to master, branch-2.2 and branch-2.1

asfgit pushed a commit that referenced this pull request Apr 28, 2017
## What changes were proposed in this pull request?

We didn't enforce analyzed plans in Spark 2.1 when writing out to Kafka.

## How was this patch tested?

New unit test.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: Bill Chambers <bill@databricks.com>

Closes #17804 from anabranch/SPARK-20496-2.

(cherry picked from commit 733b81b)
Signed-off-by: Burak Yavuz <brkyvz@gmail.com>
asfgit pushed a commit that referenced this pull request Apr 28, 2017
## What changes were proposed in this pull request?

We didn't enforce analyzed plans in Spark 2.1 when writing out to Kafka.

## How was this patch tested?

New unit test.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: Bill Chambers <bill@databricks.com>

Closes #17804 from anabranch/SPARK-20496-2.

(cherry picked from commit 733b81b)
Signed-off-by: Burak Yavuz <brkyvz@gmail.com>
@asfgit asfgit closed this in 733b81b Apr 28, 2017
jzhuge pushed a commit to jzhuge/spark that referenced this pull request Aug 20, 2018
We didn't enforce analyzed plans in Spark 2.1 when writing out to Kafka.

New unit test.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: Bill Chambers <bill@databricks.com>

Closes apache#17804 from anabranch/SPARK-20496-2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants