Skip to content

[SPARK-14874][SQL][Streaming] Remove the obsolete Batch representation #12638

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
lw-lin wants to merge 4 commits into from
Closed

[SPARK-14874][SQL][Streaming] Remove the obsolete Batch representation #12638

lw-lin wants to merge 4 commits into from

Conversation

@lw-lin
Copy link
Contributor

@lw-lin lw-lin commented Apr 23, 2016 •
edited
Loading

What changes were proposed in this pull request?

The Batch class, which had been used to indicate progress in a stream, was abandoned by [SPARK-13985][SQL] Deterministic batches with ids and then became useless.

This patch:

  • removes the Batch class
  • does some related renaming (update: this has been reverted)
  • fixes some related comments

How was this patch tested?

N/A

@SparkQA
Copy link

SparkQA commented Apr 23, 2016

Test build #56798 has finished for PR 12638 at commit c79cba9.

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

@lw-lin lw-lin closed this Apr 23, 2016
@lw-lin lw-lin changed the title [SPARK-14874][SQL][Streaming] Cleanup the useless Batch class [SPARK-14874][SQL][Streaming] Remove the obsolete Batch representation Apr 24, 2016
@lw-lin lw-lin reopened this Apr 24, 2016
@lw-lin
Copy link
Contributor Author

lw-lin commented Apr 24, 2016

@marmbrus @tdas would you mind taking a look? Thanks! :-)

@SparkQA
Copy link

SparkQA commented Apr 24, 2016

Test build #56824 has finished for PR 12638 at commit c79cba9.

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

marmbrus
marmbrus reviewed Apr 25, 2016
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/FileStreamSink.scala Outdated
private val fs = basePath.getFileSystem(sqlContext.sparkContext.hadoopConfiguration)

override def addBatch(batchId: Long, data: DataFrame): Unit = {
override def addData(batchId: Long, data: DataFrame): Unit = {
Copy link
Contributor

@marmbrus marmbrus Apr 25, 2016

Choose a reason for hiding this comment

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

We don't need to change this name. Its still a batch of data and one of the parameters is still named batch.

@marmbrus
Copy link
Contributor

marmbrus commented Apr 25, 2016

It's fine to remove the class, but lets avoid unneeded renaming.

@lw-lin
Copy link
Contributor Author

lw-lin commented Apr 26, 2016

Sure, so I'm closing this PR since the removal itself is not worthy for committers to process.
@marmbrus thanks for the review!

@lw-lin lw-lin closed this Apr 26, 2016
@marmbrus
Copy link
Contributor

marmbrus commented Apr 26, 2016

To be clear, if there's a completely unused class, I think it's worth the
time to delete it (dead code is confusing for people trying to learn the
code base).
On Apr 25, 2016 7:20 PM, "Liwei Lin" [email protected] wrote:

Closed #12638 #12638.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#12638 (comment)

@lw-lin lw-lin reopened this Apr 26, 2016
@lw-lin
Copy link
Contributor Author

lw-lin commented Apr 26, 2016 •
edited
Loading

@marmbrus thanks for the reminder!

Since I've reverted the renaming, and I've checked there's no other completely unused class in package o.a.s.sql.execution.streaming, seems this is ready to go (pending tests). So would you mind take another look? Thanks!

@SparkQA
Copy link

SparkQA commented Apr 26, 2016

Test build #56957 has finished for PR 12638 at commit 14e6900.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@lw-lin
Copy link
Contributor Author

lw-lin commented Apr 26, 2016 •
edited
Loading

some build issues unrelated to this PR

@lw-lin
Copy link
Contributor Author

lw-lin commented Apr 26, 2016

Jenkins retest this please

@SparkQA
Copy link

SparkQA commented Apr 26, 2016

Test build #56996 has finished for PR 12638 at commit 14e6900.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

lw-lin
lw-lin reviewed Apr 27, 2016
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/FileStreamSource.scala
}

/**
* Returns the next batch of data that is available after `start`, if any is available.
Copy link
Contributor Author

@lw-lin lw-lin Apr 27, 2016

Choose a reason for hiding this comment

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

This doc should be updated, following Source.getBatch()'s doc change from Returns the next batch of data that is available after start, if any is available. to Returns the data that is between the offsets (start, end].

@lw-lin
Copy link
Contributor Author

lw-lin commented Apr 27, 2016 •
edited
Loading

just rebased to master to resolve some conflicts

@SparkQA
Copy link

SparkQA commented Apr 27, 2016

Test build #57073 has finished for PR 12638 at commit 653fa52.

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

@marmbrus
Copy link
Contributor

marmbrus commented Apr 27, 2016

Thanks, merging to master.

@asfgit asfgit closed this in a234cc6 Apr 27, 2016
@lw-lin lw-lin deleted the remove-batch branch April 28, 2016 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lw-lin @SparkQA @marmbrus