Skip to content

[SPARK-25644][SS]Fix java foreachBatch in DataStreamWriter #22633

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[SPARK-25644][SS]Fix java foreachBatch in DataStreamWriter #22633

wants to merge 3 commits into from

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Oct 4, 2018

What changes were proposed in this pull request?

The java foreachBatch API in DataStreamWriter should accept java.lang.Long rather scala.Long.

How was this patch tested?

New java test.

@zsxwing
Copy link
Member Author

zsxwing commented Oct 4, 2018

cc @tdas

@zsxwing zsxwing mentioned this pull request Oct 4, 2018
Closed
HeartSaVioR
HeartSaVioR approved these changes Oct 4, 2018
View reviewed changes
Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

One minor comment (2 cents) but LGTM. Nice catch!

sql/core/src/test/java/test/org/apache/spark/sql/streaming/JavaDataStreamReaderWriterSuite.java
}

@Test
public void testForeachBatchAPI() {
Copy link
Contributor

@HeartSaVioR HeartSaVioR Oct 4, 2018

Choose a reason for hiding this comment

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

MINOR: I guess it will be duplicated effort on both Scala and Java suite, but IMHO adding sanity check wouldn't hurt much and prevent further possible misses.

@SparkQA
Copy link

SparkQA commented Oct 5, 2018

Test build #96953 has finished for PR 22633 at commit aed8c24.

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

@SparkQA
Copy link

SparkQA commented Oct 5, 2018

Test build #96955 has finished for PR 22633 at commit ce60b8a.

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

dongjoon-hyun
dongjoon-hyun reviewed Oct 5, 2018
View reviewed changes
sql/core/src/test/java/test/org/apache/spark/sql/streaming/JavaDataStreamReaderWriterSuite.java Outdated
@@ -0,0 +1,89 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
Copy link
Member

@dongjoon-hyun dongjoon-hyun Oct 5, 2018

Choose a reason for hiding this comment

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

@zsxwing . Could you fix the indentation of Apache License?

dongjoon-hyun
dongjoon-hyun reviewed Oct 5, 2018
View reviewed changes
sql/core/src/test/java/test/org/apache/spark/sql/streaming/JavaDataStreamReaderWriterSuite.java Outdated
@After
public void tearDown() {
Utils.deleteRecursively(new File(input));
spark.stop();
Copy link
Member

@dongjoon-hyun dongjoon-hyun Oct 5, 2018

Choose a reason for hiding this comment

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

Since deleteRecursively can raise IOException, can we have try ... finally ... to ensure spark.stop invocation?

HyukjinKwon
HyukjinKwon reviewed Oct 5, 2018
View reviewed changes
sql/core/src/test/java/test/org/apache/spark/sql/streaming/JavaDataStreamReaderWriterSuite.java Outdated
@Test
public void testForeachAPI() {
StreamingQuery query = spark
.readStream()
Copy link
Member

@HyukjinKwon HyukjinKwon Oct 5, 2018

Choose a reason for hiding this comment

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

Nit: 2 space indentation

HyukjinKwon
HyukjinKwon reviewed Oct 5, 2018
View reviewed changes
sql/core/src/test/java/test/org/apache/spark/sql/streaming/JavaDataStreamReaderWriterSuite.java Outdated

@Override
public void process(String value) {
}
Copy link
Member

@HyukjinKwon HyukjinKwon Oct 5, 2018

Choose a reason for hiding this comment

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

tiny nit: I would just public void process(String value) { }

@zsxwing
Copy link
Member Author

zsxwing commented Oct 5, 2018

Looks like lint-java doesn't catch any style issues in my PR

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Oct 5, 2018

Looks fine to me

@SparkQA
Copy link

SparkQA commented Oct 5, 2018

Test build #96971 has finished for PR 22633 at commit 999b6be.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Oct 5, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Oct 5, 2018

Test build #96982 has finished for PR 22633 at commit 999b6be.

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

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Oct 5, 2018

Right. It's weird that lint-java doesn't help this, @zsxwing .

dongjoon-hyun
dongjoon-hyun approved these changes Oct 5, 2018
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

@zsxwing
Copy link
Member Author

zsxwing commented Oct 5, 2018

Thanks. Merging to master and 2.4.

asfgit pushed a commit that referenced this pull request Oct 5, 2018
@zsxwing
[SPARK-25644][SS] Fix java foreachBatch in DataStreamWriter
0a70afd 
## What changes were proposed in this pull request?

The java `foreachBatch` API in `DataStreamWriter` should accept `java.lang.Long` rather `scala.Long`.

## How was this patch tested?

New java test.

Closes #22633 from zsxwing/fix-java-foreachbatch.

Authored-by: Shixiong Zhu <[email protected]>
Signed-off-by: Shixiong Zhu <[email protected]>
@asfgit asfgit closed this in 7dcc90f Oct 5, 2018
@zsxwing zsxwing deleted the fix-java-foreachbatch branch October 5, 2018 18:22
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Oct 6, 2018

It turns out that we didn't check Scala 2.12 build.

I'll make a follow-up.

jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@zsxwing
[SPARK-25644][SS] Fix java foreachBatch in DataStreamWriter
b244e8f 
## What changes were proposed in this pull request?

The java `foreachBatch` API in `DataStreamWriter` should accept `java.lang.Long` rather `scala.Long`.

## How was this patch tested?

New java test.

Closes apache#22633 from zsxwing/fix-java-foreachbatch.

Authored-by: Shixiong Zhu <[email protected]>
Signed-off-by: Shixiong Zhu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HyukjinKwon HyukjinKwon HyukjinKwon left review comments

@srowen srowen srowen approved these changes

@HeartSaVioR HeartSaVioR HeartSaVioR approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@zsxwing @SparkQA @HyukjinKwon @dongjoon-hyun @srowen @HeartSaVioR