Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-19886] Fix reportDataLoss if statement in SS KafkaSource #17228

Closed
wants to merge 6 commits into from

Conversation

brkyvz
Copy link
Contributor

@brkyvz brkyvz commented Mar 9, 2017

What changes were proposed in this pull request?

Fix the throw new IllegalStateException if statement part.

How is this patch tested

Regression test

@SparkQA
Copy link

SparkQA commented Mar 9, 2017

Test build #74275 has finished for PR 17228 at commit 1fb14aa.

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

@SparkQA
Copy link

SparkQA commented Mar 9, 2017

Test build #74276 has finished for PR 17228 at commit 5fe604b.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@marmbrus
Copy link
Contributor

marmbrus commented Mar 9, 2017

LGTM too

@SparkQA
Copy link

SparkQA commented Mar 9, 2017

Test build #74277 has finished for PR 17228 at commit 233c3f1.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Mar 9, 2017

Test build #74279 has finished for PR 17228 at commit f831360.

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

@@ -398,4 +386,23 @@ private[kafka010] object CachedKafkaConsumer extends Logging {
consumer
}
}

private def reportDataLoss0(
Copy link
Member

Choose a reason for hiding this comment

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

Why not just move reportDataLoss here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The additionalInfo requires passing a lot more variables. I can pass all of it, but yeah

Copy link
Member

Choose a reason for hiding this comment

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

I see. Then LGTM.

@zsxwing
Copy link
Member

zsxwing commented Mar 10, 2017

Merging to master and 2.1.

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

Fix the `throw new IllegalStateException` if statement part.

## How is this patch tested

Regression test

Author: Burak Yavuz <brkyvz@gmail.com>

Closes #17228 from brkyvz/kafka-cause-fix.

(cherry picked from commit 82138e0)
Signed-off-by: Shixiong Zhu <shixiong@databricks.com>
@asfgit asfgit closed this in 82138e0 Mar 10, 2017
@brkyvz brkyvz deleted the kafka-cause-fix branch February 3, 2019 20:54
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.

5 participants