Skip to content

Conversation

@srowen
Copy link
Member

@srowen srowen commented Dec 29, 2015

Replace Guava Optional with (an API clone of) Java 8 java.util.Optional (edit: and a clone of Guava Optional)

See also #10512

@SparkQA
Copy link

SparkQA commented Dec 29, 2015

Test build #48427 has finished for PR 10513 at commit 1196fcb.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • public final class Optional<T>

@SparkQA
Copy link

SparkQA commented Dec 30, 2015

Test build #48474 has finished for PR 10513 at commit 72f7122.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • public final class Optional<T> implements Serializable

@SparkQA
Copy link

SparkQA commented Dec 30, 2015

Test build #48477 has finished for PR 10513 at commit 283095b.

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

@vanzin
Copy link
Contributor

vanzin commented Dec 30, 2015

This looks ok to me if we're ok with breaking source and binary compatibility in 2.0. I guess it's the less bad option to fix this without depending on Java 8...

@rxin
Copy link
Contributor

rxin commented Dec 31, 2015

If we are creating our own Optional, why don't we also support all the methods available on Guava, so we only need to break "import" compatibility?

@srowen
Copy link
Member Author

srowen commented Dec 31, 2015

My theory was that it makes it more compatible with Java 8 Optional. Later, moving to its implementation is a smaller change. We're taking the bigger change now. Or at least that was the motivation.

Another possibility is to add the Guava work-alike methods and deprecate them. Later they'd be removed I guess when Java 8 Optional gets used. I liked that less since it still just half of the breaking change farther down the road, when a major release seems like the time to do as much of the breaking as possible.

@SparkQA
Copy link

SparkQA commented Jan 3, 2016

Test build #48605 has finished for PR 10513 at commit b0e96b4.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • public final class Optional<T> implements Serializable

@rxin
Copy link
Contributor

rxin commented Jan 3, 2016

But we might never switch to Java 8's Optional, or maybe the entire API that's depending Optional will be deprecated in the future. Why should we increase the level of breakage now in anticipation of something that we don't even know we need in the future?

@srowen
Copy link
Member Author

srowen commented Jan 3, 2016

No matter what, there's a break now -- not even source-compatible. In for a penny, in for a pound, if it has a future benefit? but I agree it's unlikely to actually switch to the Java 8 Optional as it's yet another source-incompatible change. I think it's either use Java 8 in Spark 2.x, and then really use java.util.Optional, or else go with the hybrid solution where the new class tries to implement both the Guava and Java 8 API.

@SparkQA
Copy link

SparkQA commented Jan 4, 2016

Test build #48654 has finished for PR 10513 at commit 5b3dbd0.

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

@SparkQA
Copy link

SparkQA commented Jan 6, 2016

Test build #48850 has finished for PR 10513 at commit b2c4b17.

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

@SparkQA
Copy link

SparkQA commented Jan 6, 2016

Test build #2336 has finished for PR 10513 at commit b2c4b17.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably have a better user-facing documentation in addition to this ...

@SparkQA
Copy link

SparkQA commented Jan 7, 2016

Test build #48936 has finished for PR 10513 at commit bf7a205.

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

@rxin
Copy link
Contributor

rxin commented Jan 8, 2016

LGTM. Anything else you want to do? Otherwise we should remove WIP and merge it.

@srowen
Copy link
Member Author

srowen commented Jan 8, 2016

It's ready as far as I'm concerned.

@srowen srowen changed the title [SPARK-4819] [WIP] Remove Guava's "Optional" from public API [SPARK-4819] Remove Guava's "Optional" from public API Jan 8, 2016
@rxin
Copy link
Contributor

rxin commented Jan 8, 2016

Thanks - I'm going to merge this.

@asfgit asfgit closed this in 659fd9d Jan 8, 2016
@srowen srowen deleted the SPARK-4819 branch January 8, 2016 21:10
@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/49027/
Test FAILed.

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