[SPARK-25299] Driver lifecycle api #533

yifeih · 2019-04-06T01:54:55Z

Introduce driver shuffle lifecycle APIs

yifeih · 2019-04-11T18:16:14Z

tagging @mccheah and @ifilonenko for initial review.

FYI - I searched through core spark code, but I couldn't figure out when and how the application shuffle files get removed in the case that uses YARN External Shuffle Service in cluster mode (since deleteFilesOnStop in the DiskBlockManager is false in the case of YARN ESS). The applicationRemoved() and executorRemoved() methods in ExternalShuffleBlockHandler seem only to be used in non-cluster modes. Ideally whatever codepath that is would go under the DefaultShuffleDriverComponents.cleanupApplicaiton() method

mccheah · 2019-04-11T18:27:12Z

The applicationRemoved() and executorRemoved() methods in ExternalShuffleBlockHandler seem only to be used in non-cluster modes.

Well regardless we need to make sure whatever we make is viable in client mode. Just a note.

yifeih · 2019-04-11T21:52:20Z

I'm digging through the code and I think this is how spark standalone seems to work:

Each Worker launches 1 or more Executors, which contain the spark conf and spark env (it actually launches an ExecutorRunner, which I believe seems to construct a command to launch the Executor although it wasn't automatically clear from the code). If a plugin is configured, then the external shuffle service should ideally be configured to be off, so the Executors and Driver should behave with the same code that it would in cluster mode in terms of interacting with the shuffle plugin interfaces, and the applicationRemoved() and executorRemoved() methods should do nothing. If the shuffle service is turned on, then the Worker is just launching an unused external shuffle service, which is also fine.

So I don't think we need to do anything extra to support shuffle plugins in standalone mode, but let me know if I'm missing something.

mccheah

Looks good on my end. @squito @vanzin I wonder if you can answer our inquiries that we've listed above. It's not obvious to us if we have to do anything extra than what we've done here.

mccheah · 2019-04-12T00:24:16Z

core/src/main/java/org/apache/spark/api/shuffle/ShuffleDataIO.java

@@ -28,4 +28,5 @@
 @Experimental
 public interface ShuffleDataIO {
  ShuffleExecutorComponents executor();
+  ShuffleDriverComponents driver();


Let's move driver above executor. I don't have a great reason for this except that this is how I tend to think about Spark in general though -> driver comes before executors...

mccheah · 2019-04-12T00:24:43Z

core/src/main/java/org/apache/spark/api/shuffle/ShuffleDriverComponents.java

+
+  void initializeApplication();
+
+  void cleanupApplication() throws IOException;


Any reason for this to throw IOException? Think we can go without any throws declaration for now.

Hmm i guess i was expecting this to include something that would make a call somewhere or hit the file system? I can remove it for now

squito · 2019-04-12T20:43:10Z

I couldn't figure out when and how the application shuffle files get removed in the case that uses YARN External Shuffle Service

YarnShuffleService is runs within yarn's Nodemanager, and its stopApplication() method gets called by yarn when yarn stops the application -- you won't see it getting called directly from spark code. that in turn calls blockHandler.applicationRemoved().

squito

minor stuff, but looks fine to me. I think @vanzin might have stronger opinions about it, I think he has thought more about how a spark app would register with an external service. But we can also iterate on this later as well if you want to get it in.

squito · 2019-04-12T20:48:17Z

core/src/main/java/org/apache/spark/shuffle/sort/lifecycle/DefaultShuffleDriverComponents.java

+  @Override
+  public void initializeApplication() {
+    blockManagerMaster = SparkEnv.get().blockManager().master();
+  }


I guess the idea is that for a truly external shuffle service, here the driver would register with that service (including any authentication etc.) and setup heartbeats?

Yeah, that would be for non-default implementations, like the external implementation I experimented with in my old PR

squito · 2019-04-12T20:49:50Z

core/src/main/java/org/apache/spark/api/shuffle/ShuffleDataCleaner.java

+
+import java.io.IOException;
+
+public interface ShuffleDataCleaner {


does this really need to be separate from ShuffleDriverComponents? just seems to add more levels of indirection to track.

I guess it doesn't but I prefer it this way because it does emphasize that the cleaner runs only on the driver

I mean, can't you just put removeShuffleData() directly in ShuffleDriverComponents, and then get rid of this interface completely? I think it would still be obvious it runs only on the driver.

vanzin

Probably out of scope for this change, but I have a feeling that for non-default configs it would be useful to have a way for the driver component to provide data to the executor components.

If the plugin is inside Spark it can probably hook into other means of doing it (like using the RPC classes), but if the goal is to support code living outside Spark, that would be a little more awkward.

So maybe, e.g. initializeApplication could return something that is transferred by Spark to executors and provided to the initialization method of ShuffleExecutorComponents. Or, if the SparkConf is provided to the driver initialization, it could be used to set configs for executors to use. In any case, I think it would be good to make the mechanism clear so that implementations don't need to reinvent it themselves.

vanzin · 2019-04-18T18:25:06Z

core/src/main/java/org/apache/spark/api/shuffle/ShuffleDriverComponents.java

+
+public interface ShuffleDriverComponents {
+
+  void initializeApplication();


Should you provide some parameter here? At least SparkConf seems like it would be useful.

(You can probably get it from SparkEnv but I always feel dirty calling that class.)

ShuffleDataIO is service-loaded by Spark, so its constructor can take a SparkConf as an argument. However, I've never been certain on if we should rely on that vs. passing the SparkConf directly as an argument to initialization methods. The same can be said about executor components initializing executors. Thoughts?

If that behavior is documented here then it should be fine. Either one works.

vanzin · 2019-04-18T18:27:10Z

core/src/main/scala/org/apache/spark/SparkContext.scala

+    val maybeIO = Utils.loadExtensions(
+      classOf[ShuffleDataIO], Seq(configuredPluginClasses), conf)
+    require(maybeIO.size == 1, s"Failed to load plugins of type $configuredPluginClasses")
+    _shuffleDriverComponents = maybeIO.head.driver


mccheah · 2019-04-18T22:43:56Z

Probably out of scope for this change, but I have a feeling that for non-default configs it would be useful to have a way for the driver component to provide data to the executor components.

Does accepting SparkConf in the constructor of ShuffleDataIO suffice? Or did we need something more sophisticated, for example the driver discovers some set of available remote servers and communicates those over to the executor components?

vanzin · 2019-04-18T22:46:57Z

Or did we need something more sophisticated, for example the driver discovers some set of available remote servers and communicates those over to the executor components?

That's pretty much the use case I'm thinking of. It could be done through SparkConf but then it puts a burden on the configuration object being propagated properly so that the changes made by the driver plugin are visible to the executors when they start up. It may be the case already, haven't really tracked all the code.

mccheah · 2019-04-18T22:50:49Z

That kind of protocol might belong in the implementation of the plugin and doesn't necessarily belong in core Spark as an RPC API. But perhaps if enough implementations end up wanting to do similar things down the line then we can support it in a first class way.

vanzin · 2019-04-18T22:57:27Z

That kind of protocol might belong in the implementation of the plugin

Maybe, but even then, how would executors know how to connect back to the driver endpoint of the plugin? For example, for socket communication, they may be able to get the driver address from Spark's conf, but which port?

There must be some kind of communication between driver and executor to bootstrap that process. Not all plugins will be able to deal with static configuration (i.e. stuff you can pass to spark-submit with --conf).

mccheah · 2019-04-18T23:06:44Z

What would be the API and the hooks we'd have to add to set up that communication? I'm very reluctant to add the complexity of having the driver serialize a bundle up to the executors via custom RPC endpoints set up on both sides.

vanzin · 2019-04-18T23:08:19Z

I think making sure that the driver plugin can modify SparkConf, and that those modifications are available on the executor side when the executor plugin is started is enough to bootstrap this. No need to get too fancy.

mccheah · 2019-04-18T23:10:17Z

How does the driver pass specific Spark configuration up to the executors? I thought once you started a Spark Context with a given Spark configuration, the executors are tied down to only using said configuration. Would we have to hook into the cluster manager specific scheduler backends? E.g. the Kubernetes executor pod launcher knows to pass along certain Spark configurations when launching executors.

vanzin · 2019-04-18T23:16:28Z

Any config modification that is made on the driver side before the scheduler backend is started ends up propagated to executors. So if this plugin is being initialized before the scheduler backend is started, then the configuration should make it to executors. On the executor side, as long as the executor plugin is being initialized after the executor sends the RetrieveSparkAppConfig message to the driver, the plugin should see them.

Otherwise, you'd probably have to create another RPC to get the shuffle plugin-specific configs and make them available to the executor.

squito · 2019-04-19T02:18:06Z

btw this topic of exchanging communication info also came up in a discussion on the executor plugin: https://issues.apache.org/jira/browse/SPARK-24918?focusedCommentId=16572850&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16572850

yifeih · 2019-04-19T20:40:59Z

I don't think every plugin will need to setup communication between the driver and executor (in the case of kubernetes, the executors can dynamically discover the list of shuffle servers themselves), but I see how the current APIs would make setting sparkConf seem a bit more like a hack. Since the driver initialization happens in setting up the SparkContext, we could make initializeApplication() return a map of additional SparkConf configs that could be set in the SparkContext initialization code? And then move the plugin initialization to before the task scheduler starts. Would that address the driver to executor communication?

vanzin · 2019-04-19T23:12:02Z

we could make initializeApplication() return a map of additional SparkConf configs that could be set in the SparkContext initialization code?

That sounds good enough for me.

mccheah · 2019-04-19T23:14:38Z

Let's see how much work that would take - but if it looks to be too involved I'd like to make that a separate patch. We'd also have to test it against at least some of the cluster managers specifically.

mccheah · 2019-04-19T23:18:26Z

Namely because I think writing an integration test that validates the behavior will not be easy to write.

vanzin · 2019-04-19T23:22:20Z

Should be pretty simple e.g. in YarnClusterSuite. (Could also try using local-cluster[...] as the master.)

yifeih · 2019-04-20T04:42:13Z

core/src/test/scala/org/apache/spark/shuffle/ShuffleDriverComponentsTest.scala

+
+class TestShuffleDriverComponents extends ShuffleDriverComponents {
+  override def initializeApplication(): util.Map[String, String] =
+    ImmutableMap.of("spark.test.key", "spark.test.value")


For some reason, this test only works with spark conf settings that start with spark.... i wasn't able to find out why, but i'll dig more after the weekend.

yifeih · 2019-04-29T22:07:31Z

@vanzin i searched for a while, but I couldn't find the location in the code that limits the spark configs passed from the driver to the executor to start with the spark. prefix. The test breaks if I remove the spark. prefix on the key. I'm trying to figure out if it's a result of local-cluster deploy mode or something else, and whether we should just prefix all the driver configs returned with spark. automatically for the plugin implementer.

yifeih · 2019-04-29T22:42:51Z

Actually, another suggestion would be to require a prefix and check for that prefix upon return from the initializeApplication() method (i.e. spark.shuffle.plugin)

vanzin · 2019-04-29T22:46:39Z

I'm pretty sure the check is in CoarseGrainedSchedulerBackend.scala, check where sparkProperties is initialized.

What you could do, also to make the API more symmetric, is to add the prefix yourself when stashing the driver config in the SparkConf, and remove it on the executor side before passing a Map[String, String] to its initialization method (instead of relying on the code reading it from SparkConf directly).

mccheah · 2019-05-01T00:41:05Z

This is looking close. @vanzin can you sanity check our implementation of properties propagation and confirm that it's in line with what you suggested? I'm +1 on my end.

vanzin

Looks fine.

vanzin · 2019-05-01T01:31:54Z

core/src/main/scala/org/apache/spark/SparkContext.scala

+      classOf[ShuffleDataIO], Seq(configuredPluginClasses), conf)
+    require(maybeIO.size == 1, s"Failed to load plugins of type $configuredPluginClasses")
+    _shuffleDriverComponents = maybeIO.head.driver
+    _shuffleDriverComponents.initializeApplication().asScala.foreach(x =>


.foreach { case (k, v) =>

vanzin · 2019-05-01T01:32:33Z

core/src/main/scala/org/apache/spark/shuffle/sort/SortShuffleManager.scala

@@ -219,7 +221,13 @@ private[spark] object SortShuffleManager extends Logging {
      classOf[ShuffleDataIO], Seq(configuredPluginClasses), conf)
    require(maybeIO.size == 1, s"Failed to load plugins of type $configuredPluginClasses")
    val executorComponents = maybeIO.head.executor()
-    executorComponents.initializeExecutor(conf.getAppId, SparkEnv.get.executorId)
+    val extraConfigs = conf.getAllWithPrefix(ShuffleDataIO.SHUFFLE_SPARK_CONF_PREFIX)
+        .map( e => (e._1.stripPrefix(ShuffleDataIO.SHUFFLE_SPARK_CONF_PREFIX), e._2))


Almost sure this is not needed. If it is, use the style I suggested above.

vanzin · 2019-05-01T01:34:54Z

core/src/test/scala/org/apache/spark/shuffle/ShuffleDriverComponentsSuite.scala

+class TestShuffleExecutorComponents(sparkConf: SparkConf) extends ShuffleExecutorComponents {
+  override def initializeExecutor(appId: String, execId: String,
+                                  extraConfigs: util.Map[String, String]): Unit = {
+    assert(extraConfigs.get("test-key").equals("test-value"))


== instead of equals (which also gives a better error instead of an NPE if something's wrong)

squito

just some nits but otherwise lgtm

squito · 2019-05-01T19:21:37Z

core/src/main/scala/org/apache/spark/SparkContext.scala

+    val maybeIO = Utils.loadExtensions(
+      classOf[ShuffleDataIO], Seq(configuredPluginClasses), conf)
+    require(maybeIO.size == 1, s"Failed to load plugins of type $configuredPluginClasses")
+    _shuffleDriverComponents = maybeIO.head.driver


nit: since you're calling a java method, I prefer with the parens driver()

squito · 2019-05-01T19:22:09Z

core/src/test/scala/org/apache/spark/InternalAccumulatorSuite.scala

@@ -210,7 +210,8 @@ class InternalAccumulatorSuite extends SparkFunSuite with LocalSparkContext {
  /**
   * A special [[ContextCleaner]] that saves the IDs of the accumulators registered for cleanup.
   */
-  private class SaveAccumContextCleaner(sc: SparkContext) extends ContextCleaner(sc) {
+  private class SaveAccumContextCleaner(sc: SparkContext) extends
+    ContextCleaner(sc, null) {


nit: indent more

mccheah · 2019-05-07T23:38:33Z

Think all the comments are addressed so far. I'm going to merge this to unblock Ignite shuffle, otherwise the caches are going to be bloated indefinitely =) thanks everyone for reviewing!

jerryshao · 2019-06-13T07:24:58Z

core/src/test/scala/org/apache/spark/shuffle/ShuffleDriverComponentsSuite.scala

+import org.apache.spark.shuffle.sort.io.DefaultShuffleWriteSupport
+
+class ShuffleDriverComponentsSuite extends SparkFunSuite with LocalSparkContext {
+  test(s"test serialization of shuffle initialization conf to executors") {


string interpolation s is not necessary here.

Introduce driver shuffle lifecycle APIs

@SuppressWarnings

###### _excavator_ is a bot for automating changes across repositories. Changes produced by the roomba/latest-baseline check. # Release Notes ## 0.50.0 [feature] Warn against .parallel() calls on Java streams (#537) [fix] Correct prioritisation of versions.props to match nebula logic (#533) ## 0.51.0 - [feature] New 'com.palantir.baseline-reproducibility' plugin (#539) - [improvement] `./gradlew idea` deletes redundant ipr files (#550) - [fix] ValidateConstantMessage error-prone check is more accurate. (#546) ## 0.51.1 - [fix] Fix cleanup of old idea project files (#559) - [fix] Remove stale references to no longer existing puppycrawl checkstyle DTDs (#556) ## 0.52.0 - [improvement] errorprone 2.3.2 -> 2.3.3 (#561) - [feature] new plugin: `com.palantir.baseline-exact-dependencies` helps declare necessary and sufficient dependencies (#548) - [improvement] Split out circle style plugin into generic junit reports plugin #564 ## 0.53.0 - [improvement] Disallow javafx imports with checkstyle #569 - [fix] Avoid lambda to allow build caching of checkstyle results #576 ## 0.54.0 - [feature] New `com.palantir.baseline-release-compatibility` plugin (#582) ## 0.55.0 [break] Enable running of unique class check on multiple configurations (#583) ## 0.55.1 [fix] checkImplicitDependencies shouldn't count ignored artifacts (#601) ## 0.55.2 [fix] BaselineReleaseCompatibility up-to-date checking of compile tasks (#605) ## 0.56.0 [feature] Add an errorprone rule GradleCacheableTaskAction that prevents passing a lambda to Task.doFirst or Task.doLast when implementing gradle plugins (#608) ## 0.57.0 * [feature] Error prone rule to replace `Iterables.partition(List, int)` with `Lists.partition(List, int)` (#622) * [feature] Error prone rule to prefer `Lists` or `Collections2` `transfrom` over `Iterables.transform` (#623) ## 0.58.0 [improvement] make CheckClassUniquenessTask cacheable (#637) [fix] Add Javac Settings to uncheck "Use compiler from module target JDK when possible" (#629) [fix] class uniqueness rule must have a config (#638) ## 0.59.0 [improvement] Spotless to remove blank lines at start of methods (#641) ## 0.60.0 * [improvement] New PreferBuiltInConcurrentKeySet suggestion (#649) * [improvement] Start publishing plugin to the [Gradle plugin portal](https://plugins.gradle.org/plugin/com.palantir.baseline) (#613) ## 0.61.0 - [improvement] Sensible defaults for test tasks (HeapDumpOnOutOfMemory) (#652) ## 0.62.0 * [improvement] Ensure Optional#orElse argument is not method invocation (#655) ## 0.62.1 [fix] Revert "[improvement] Ensure Optional#orElse argument is not method invocation" (#659) ## 0.63.0 [improvement] Support auto-applying error-prone suggested fixes (#660) ## 0.64.0 * [improvement] Refaster rule compilation (#661) ## 0.64.1 - [improvement] JUnit 5 boilerplate #666 ## 0.65.0 [improvement] Error-prone check to help prevent logging AuthHeader and BearerToken (#654) [fix] fix potential NPE when configuring testing (#669) [fix] Fix refaster compilation to support version recommendations (#667) ## 0.66.0 [improvement] Ignore DesignForExtension for ParameterizedTest (#673) ## 0.66.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | The PreventTokenLogging error-prone check will now correctly handle null use in SLF4J and Safe/Unsafe Arg functions. | palantir/gradle-baseline#674 | ## 1.0.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Add refaster rule to migrate away from optional.orElse(supplier.get()) | palantir/gradle-baseline#679 | | Fix | Projects can now compile using Java12, because the one errorprone check that breaks (Finally) is now disabled when you use this toolchain. It remains enabled when compiling against earlier JDKs. | palantir/gradle-baseline#681 | ## 1.1.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Ensure that format tasks execute after compilation | palantir/gradle-baseline#688 | ## 1.1.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Auto-fix OptionalOrElseMethodInvocation using `-PerrorProneApply`. | palantir/gradle-baseline#690 | ## 1.2.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Spotless check for disallowing dangling parenthesis. | palantir/gradle-baseline#687 | ## 1.3.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Don't cache test tasks in the build cache by default. It's possible to restore caching by adding `com.palantir.baseline.restore-test-cache = true` to your `gradle.properties`. | palantir/gradle-baseline#694 | ## 1.4.0 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | No longer cache javaCompile tasks when applying errorprone or refaster checks. | palantir/gradle-baseline#696 | | Feature | Test helper for refaster checks. | palantir/gradle-baseline#697 | ## 1.5.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Determine whether to use junitPlatform on a per source set basis | palantir/gradle-baseline#701 | | Feature | OptionalOrElseMethodInvocation now checks for constructor invocations. | palantir/gradle-baseline#702 | ## 1.6.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | The severity of PreferSafeLoggableExceptions and PreferSafeLoggingPreconditions is now WARNING. | palantir/gradle-baseline#704 | | Fix | OptionalOrElseMethodInvocation now allows method references in orElse. | palantir/gradle-baseline#709 | ## 1.6.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Do not overwrite user provided test configure when using junit5 | palantir/gradle-baseline#712 | ## 1.7.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Baseline can now re-format all your Java files using the Eclipse formatter. This is currently an opt-in preview, try it out by running `./gradlew format -Pcom.palantir.baseline-format.eclipse`. | palantir/gradle-baseline#707 | | Improvement | Add errorprone check to ensure junit5 tests are not used with junit4 Rule/ClassRule | palantir/gradle-baseline#714 | ## 1.8.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Checkstyle now tolerates empty lambda bodies (e.g. `() -> {}` | palantir/gradle-baseline#715 | ## 1.8.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Correctly set dependency between spotlessApply and baselineUpdateConfig to prevent a race | palantir/gradle-baseline#724 | ## 1.8.2 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Correctly handle `EnableRuleMigrationSupport` in `JUnit5RuleUsage` errorprone-rule | palantir/gradle-baseline#725 | ## 1.9.0 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Wrap long parameterized types where necessary | palantir/gradle-baseline#716 | | Improvement | Allow suppression of the TODO checkstyle check by giving it an ID. Clarify its comment to allow // TODO(username): ... | palantir/gradle-baseline#727 | | Improvement | IntelliJ GitHub issue navigation | palantir/gradle-baseline#729 | | Improvement | print out suggestion for module dependencies inclusion in useful format | palantir/gradle-baseline#733 | | Fix | The `checkImplicitDependencies` task will no longer suggest a fix of the current project. | palantir/gradle-baseline#736, palantir/gradle-baseline#567 | | Improvement | Implement DangerousCompletableFutureUsage errorprone check | palantir/gradle-baseline#740 | ## 1.10.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Refaster to use `execute` over `submit` when the result is ignored | palantir/gradle-baseline#741 | ## 1.10.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Enable applying refaster rules for repos with -Xlint:deprecation | palantir/gradle-baseline#742 | ## 1.11.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Apply `InputStreamSlowMultibyteRead` error prone check at ERROR severity | palantir/gradle-baseline#749 | ## 1.12.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | The `baseline-idea` plugin now generates configuration more closely aligned with Gradle defaults. | palantir/gradle-baseline#718 | | Improvement | Apply the suggested fixes for `UnusedMethod` and `UnusedVariable`. | palantir/gradle-baseline#751 | | Improvement | Refaster `stream.sorted().findFirst()` into `stream.min(Comparator.naturalOrder())` | palantir/gradle-baseline#752 | | Improvement | Error prone validation that Stream.sort is invoked on comparable streams | palantir/gradle-baseline#753 | | Improvement | `DangerousStringInternUsage`: Disallow String.intern() invocations | palantir/gradle-baseline#754 | ## 1.12.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Do not apply the suggested fixes for `UnusedMethod` and `UnusedVariable` which automaticall remove code with side effects. | palantir/gradle-baseline#757 | ## 1.13.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Remove errorprone `LogSafePreconditionsConstantMessage` | palantir/gradle-baseline#755 | | Improvement | Disable errorprone `Slf4jLogsafeArgs` in test code | palantir/gradle-baseline#756 | | Improvement | error-prone now detects `Duration#getNanos` mistakes and bans URL in equals methods | palantir/gradle-baseline#758 | ## 1.14.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Implement `OptionalOrElseThrowThrows` to prevent throwing from orElseThrow | palantir/gradle-baseline#759 | ## 1.15.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | LogSafePreconditionsMessageFormat disallows slf4j-style format characters | palantir/gradle-baseline#761 | | Improvement | Error Prone LambdaMethodReference check | palantir/gradle-baseline#763 | ## 1.16.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | baseline-circleci no longer integrates with the (deprecated) FindBugs plugin, as a pre-requisite for Gradle 6.0 compatibility. | palantir/gradle-baseline#766 | ## 1.17.0 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | The `TypeParameterUnusedInFormals` errorprone check is disabled when compiling on Java 13, to workaround an error-prone bug. | palantir/gradle-baseline#767 | | Improvement | Publish scm information within POM | palantir/gradle-baseline#769 | ## 1.17.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | LambdaMethodReference avoids suggestions for non-static methods | palantir/gradle-baseline#771 | ## 1.17.2 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Remove pom only dependencies from analysis in checkUnusedDependencies | palantir/gradle-baseline#773 | ## 1.18.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | When computing unused dependencies, compileOnly and annotationProcessor dependencies are ignored due to false positives as these dependencies will not appear as dependencies in the generated byte-code, but are in fact necessary dependencies to compile a given module. | palantir/gradle-baseline#783 | ## 1.19.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Disable `PreconditionsConstantMessage` on gradle plugins | palantir/gradle-baseline#790 | ## 2.0.0 | Type | Description | Link | | ---- | ----------- | ---- | | Break | Add gradle 6.0-20190904072820+0000 compatibiltiy. This raises minimum required version of gradle for plugins from this repo to 5.0. | palantir/gradle-baseline#791 | ## 2.1.0 | Type | Description | Link | | ---- | ----------- | ---- | | Feature | Automatically configure the [Intellij Eclipse format plugin](https://plugins.jetbrains.com/plugin/6546-eclipse-code-formatter) to use the eclipse formatter | palantir/gradle-baseline#794 | ## 2.1.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Stop applying error prone patches for checks that have been turned off. | palantir/gradle-baseline#793 | ## 2.2.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | baseline-circleci now validates that the rootProject.name isn't the CircleCI default (`project`) as can interfere with publishing. | palantir/gradle-baseline#775 | | Improvement | Remove JGit dependency | palantir/gradle-baseline#798 | ## 2.2.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Don't add whitespace to blank lines inside comments. Fixes apache#799 | palantir/gradle-baseline#800 | | Fix | Eclipse formatter now aligns multicatch so that it passes checkstyle. | palantir/gradle-baseline#807 | ## 2.2.2 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | ClassUniquenessPlugin now checks the `runtimeClasspath` configuration by default. | palantir/gradle-baseline#810 | ## 2.3.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | SafeLoggingExceptionMessageFormat disallows `{}` in safelog exception messages | palantir/gradle-baseline#815 | ## 2.4.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | A new `StrictUnusedVariable` check will catch any unused arguments (e.g. AuthHeaders) to public methods. If you need to suppress this, rename your variable to have an underscore prefix (e.g. `s/foo/_foo/`) or just run `./gradlew classes -PerrorProneApply` to auto-fix | palantir/gradle-baseline#819 | | Improvement | Message format checks use instanceof rather than catching | palantir/gradle-baseline#821 | ## 2.4.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Avoid false positives caused by `module-info.class` when checking class uniqueness | palantir/gradle-baseline#823 | ## 2.4.2 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Checkstyle tasks only check their own source set and only actual java sources. They don't look in your `src/*/resources` directory anymore. | palantir/gradle-baseline#830 | ## 2.4.3 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Add link to StrictUnusedVariable that directs users to baseline repo. | palantir/gradle-baseline#829 | | Fix | Long try-with-resources statements are now aligned such that the first assignment stays on the first line. | palantir/gradle-baseline#835 | ## 2.5.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Error Prone StringBuilderConstantParameters. StringBuilder with a constant number of parameters should be replaced by simple concatenation. The Java compiler (jdk8) replaces concatenation of a constant number of arguments with a StringBuilder, while jdk 9+ take advantage of JEP 280 (https://openjdk.java.net/jeps/280) to efficiently pre-size the result for better performance than a StringBuilder. | palantir/gradle-baseline#832 | ## 2.6.0 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Excavator PRs that apply other refaster rules (e.g. Witchcraft ones) will not also apply baseline refaster rules. | palantir/gradle-baseline#827 | | Improvement | Added a new ErrorProne check `PreferAssertj` to assist migration to AssertJ from legacy test frameworks. It may be necessary to add a dependency on `org.assertj:assertj-core` in modules which do not already depend on AssertJ. If there's a technical reason that AssertJ cannot be used, `PreferAssertj` may be explicitly disabled to prevent future upgrades from attempting to re-run the migration. | palantir/gradle-baseline#841 | ## 2.7.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | `StrictUnusedVariable` now ignores variables prefixed with `_` and the suggested fix will rename all unused parameters in public methods instead of removing them | palantir/gradle-baseline#833 | | Improvement | ErrorProne will now detect dangerous usage of `@RunWith(Suite.class)` that references JUnit5 classes, as this can cause tests to silently not run! | palantir/gradle-baseline#843 | ## 2.8.0 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | PreferAssertj provides better replacements fixes | palantir/gradle-baseline#850 | | Improvement | Do not run error prone on any code in the build directory | palantir/gradle-baseline#853 | ## 2.8.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Fix hamcrest arrayContainingInAnyOrder conversion | palantir/gradle-baseline#859 | ## 2.9.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | StrictUnusedVariable can only be suppressed with `_` prefix | palantir/gradle-baseline#854 | | Improvement | StrictUnusedVariable is now an error by default | palantir/gradle-baseline#855 | | Fix | The PreferAssertj refactoring will only be applied if you have explicitly opted in (e.g. using `baselineErrorProne { patchChecks += 'PreferAssertj' }` | palantir/gradle-baseline#861 | ## 2.9.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Error prone will correctly ignore all source files in the build directory and in any generated source directory | palantir/gradle-baseline#864 | | Fix | Ensure that `StrictUnusedVariable` correctly converts previously suppressed variables `unused` to `_` | palantir/gradle-baseline#865 | ## 2.9.2 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | When removing unused variables, `StrictUnusedVariable` will preserve side effects | palantir/gradle-baseline#870 | ## 2.10.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | A new `checkJUnitDependencies` task detects misconfigured JUnit dependencies which could result in some tests silently not running. | palantir/gradle-baseline#837 | | Improvement | Some AssertJ assertions can now be automatically replaced with more idiomatic ones using refaster. | palantir/gradle-baseline#851 | | Fix | PreferAssertj check avoids ambiguity in assertThat invocations | palantir/gradle-baseline#874 | | Improvement | Improve performannce of error prone PreferAssertj check | palantir/gradle-baseline#875 | | Improvement | StringBuilderConstantParameters suggested fix doesn't remove comments | palantir/gradle-baseline#877 | ## 2.10.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Allow junit4 dependencies to exist without junit4 tests | palantir/gradle-baseline#880 | ## 2.11.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | PreferAssertj supports migration of zero-delta floating point array asserts | palantir/gradle-baseline#883 | ## 2.11.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | checkJunitDependencies only checks Java source | palantir/gradle-baseline#885 | ## 2.11.2 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | AssertJ Refaster fixes use static `assertThat` imports | palantir/gradle-baseline#887 | ## 2.12.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Disable `UnusedVariable` error prone rule by default | palantir/gradle-baseline#888 | ## 2.13.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Refaster for AssertJ isZero/isNotZero/isOne and collections | palantir/gradle-baseline#881 | | Improvement | AssertJ refaster migrations support string descriptions | palantir/gradle-baseline#891 | | Fix | Certain error-prone checks are disabled in test code, and the presence of JUnit5's `@TestTemplate` annotation is now used to detect whether a class is test code. | palantir/gradle-baseline#892 | | Fix | BaselineFormat task exclude generated code on Windows | palantir/gradle-baseline#896 | ## 2.14.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Refaster rules for AssertJ tests | palantir/gradle-baseline#898 | | Improvement | refaster replacement for assertj hasSize(foo.size) -> hasSameSizeAs | palantir/gradle-baseline#900 | | Fix | Keep spotless plugin from eagerly configuring all tasks | diffplug/spotless#444 | | Fix | Continue when RefasterRuleBuilderScanner throws | palantir/gradle-baseline#904 | | Improvement | Refaster now works on repos using Gradle 6.0 | palantir/gradle-baseline#804, palantir/gradle-baseline#906 | ## 2.15.0 _No documented user facing changes_ ## 2.16.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Rewrite ImmutableCollection#addAll to add for arrays | palantir/gradle-baseline#743 | | Improvement | Add refaster rule to simplify empty optional asserts | palantir/gradle-baseline#911 | | Improvement | Baseline now allows static imports of AssertJ and Mockito methods. | palantir/gradle-baseline#915 | | Improvement | Remove refaster AssertjIsOne rule. | palantir/gradle-baseline#917 | | Improvement | Add assertj refaster rules for map size asserts | palantir/gradle-baseline#919 | | Improvement | Added a Refaster rule to change `isEqualTo` checks into `hasValue` checks | | ## 2.17.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Implement AssertjCollectionHasSameSizeAsArray | palantir/gradle-baseline#922 | | Improvement | Implement assertj map refactors for containsKey and containsEntry | palantir/gradle-baseline#925 | | Improvement | Refaster assertj migrations support descriptions with format args | palantir/gradle-baseline#926 | | Improvement | Refaster out String.format from describedAs | palantir/gradle-baseline#927 | ## 2.18.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Refaster rules to simplify negated boolean expressions and extract null checks. | palantir/gradle-baseline#935 | | Improvement | Refaster rules for checks that maps do not contain a specific key | palantir/gradle-baseline#935 | | Improvement | Refaster rule 'CollectionStreamForEach' | palantir/gradle-baseline#942 | | Improvement | ExecutorSubmitRunnableFutureIgnored as error prone ERROR | palantir/gradle-baseline#943 | ## 2.19.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | checkJUnitDependencies detects a possible misconfiguration with spock and JUnit5 which could lead to tests silently not running. | palantir/gradle-baseline#951 | ## 2.20.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Use Mockito verifyNoInteractions over deprecated verifyZeroInteractions | palantir/gradle-baseline#924 | | Improvement | Errorprone rules for usage of Guava static factory methods | palantir/gradle-baseline#941 | | Improvement | Fix error-prone `UnnecessaryParentheses` by default | palantir/gradle-baseline#952 | | Improvement | Implement Error Prone `ThrowError` to discourage throwing Errors in production code Errors are often handled poorly by libraries resulting in unexpected behavior and resource leaks. It's not obvious that 'catch (Exception e)' does not catch Error. This check is intended to be advisory - it's fine to `@SuppressWarnings("ThrowError")` in certain cases, but is usually not recommended unless you are writing a testing library that throws AssertionError. | palantir/gradle-baseline#957 | | Improvement | Improve TestCheckUtils.isTestCode test detection | palantir/gradle-baseline#958 | | Improvement | Implement Error Prone `Slf4jLevelCheck` to validate that slf4j level checks agree with contained logging. | palantir/gradle-baseline#960 | ## 2.20.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Suppress error-prone PreferCollectionConstructors on jdk13 | palantir/gradle-baseline#968 | ## 2.21.0 | Type | Description | Link | | ---- | ----------- | ---- | | Feature | Users can opt-in to format their files using our fork of google-java-format (palantir-java-format) | palantir/gradle-baseline#936 | ## 2.22.0 _Automated release, no documented user facing changes_ ## 2.23.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Implement error prone ReverseDnsLookup for unexpected reverse dns lookups Calling address.getHostName may result in a DNS lookup which is a network request, making the invocation significantly more expensive than expected depending on the environment. This check is intended to be advisory - it's fine to @SuppressWarnings("ReverseDnsLookup") in certain cases, but is usually not recommended. | palantir/gradle-baseline#970 | ## 2.24.0 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | The deprecated `verifyZeroInteractions` now gets rewritten to `verifyNoMoreInteractions`, which has the same behaviour. | palantir/gradle-baseline#975 | | Improvement | ReadReturnValueIgnored: Check that read operation results are not ignored | palantir/gradle-baseline#978 | | Improvement | Stop migrating source sets to safe-logging, unless they already have the requisite library (`com.palantir.safe-logging:preconditions`). | palantir/gradle-baseline#981 | | Improvement | For users who opted into palantir-java-format, we now reflow strings and reorder imports. | palantir/gradle-baseline#982 | ## 2.25.0 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | checkstyle Indentation rule is disabled when palantir-java-format is enabled | palantir/gradle-baseline#987 | | Improvement | Load palantir-java-format dynamically from the same configuration set up by `com.palantir-java-format` which is also used to determine the version used by IntelliJ. | palantir/gradle-baseline#989 | ## 2.26.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Run `./gradlew formatDiff` to reformat the relevant sections of any uncommitted changed Java files (relies on `git diff -U0 HEAD` under the hood) | palantir/gradle-baseline#988 | ## 2.27.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Slf4jLogsafeArgs fixes safe-log wrapped throwables | palantir/gradle-baseline#1001 | | Improvement | `DangerousParallelStreamUsage` checks for `Collection.parallelStream()` and `StreamSupport` utility methods with parallel=true. | palantir/gradle-baseline#1005 | | Improvement | DangerousThrowableMessageSafeArg disallows Throwables in SafeArg values. Throwables must be logged without an Arg wrapper as the last parameter, otherwise unsafe data may be leaked from the unsafe message or the unsafe message of a cause. | palantir/gradle-baseline#997 | | Improvement | Implement a suggested fix for CatchBlockLogException | palantir/gradle-baseline#998 | ## 2.28.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Implement `FinalClass` error prone check, replacing the checkstyle implementation | palantir/gradle-baseline#1008 | | Improvement | Error prone validation to avoid redundant modifiers | palantir/gradle-baseline#1010 | ## 2.28.1 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Fix `RedundantModifier` interpretation of implicit modifiers | palantir/gradle-baseline#1014 | ## 2.28.2 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Fix RedundantModifier failures types nested in interfaces | palantir/gradle-baseline#1017 | ## 2.28.3 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Fix error-prone mathcing literal null as a subtype. The most common issue this fixes is failures on `SafeArg.of("name", null)` assuming that the null literal value parameter may be a throwable. | palantir/gradle-baseline#1020 | To enable or disable this check, please contact the maintainers of Excavator.

yifeih added 12 commits April 5, 2019 11:50

add api

bebeea4

initial impl

f6e84de

initial impl

6f41113

fix ups

0d0f368

scalastyle

6cb592a

style

8d362b9

fix compile?

d8194a4

style

4ee7954

oops

b370172

add blocking flag back

111d5ad

cleanup

484cf3f

remove unused imports

0a94436

mccheah reviewed Apr 12, 2019

View reviewed changes

squito reviewed Apr 12, 2019

View reviewed changes

address comments

2dd5519

vanzin reviewed Apr 18, 2019

View reviewed changes

add sparkconf setting in driver and test

29b9a6c

yifeih commented Apr 20, 2019

View reviewed changes

fix tests?

89d0a6c

pass extra configs through interface

121c112

vanzin reviewed May 1, 2019

View reviewed changes

squito approved these changes May 1, 2019

View reviewed changes

address comments

6dc0a24

mccheah approved these changes May 7, 2019

View reviewed changes

bulldozer-bot bot merged commit ab9131d into spark-25299 May 7, 2019

bulldozer-bot bot deleted the yh/lifecycle branch May 7, 2019 23:38

jerryshao reviewed Jun 13, 2019

View reviewed changes

yifeih added a commit that referenced this pull request Oct 4, 2019

[SPARK-25299] Driver lifecycle api (#533)

0ac7fac

Introduce driver shuffle lifecycle APIs

svc-excavator-bot mentioned this pull request Nov 6, 2019

Excavator: Upgrades Baseline to the latest version #621

Merged


		void initializeApplication();

		void cleanupApplication() throws IOException;


		import java.io.IOException;

		public interface ShuffleDataCleaner {


		public interface ShuffleDriverComponents {

		void initializeApplication();

[SPARK-25299] Driver lifecycle api #533

[SPARK-25299] Driver lifecycle api #533

Conversation

yifeih commented Apr 6, 2019

yifeih commented Apr 11, 2019

mccheah commented Apr 11, 2019

yifeih commented Apr 11, 2019

mccheah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squito commented Apr 12, 2019

squito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifilonenko Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanzin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccheah commented Apr 18, 2019

vanzin commented Apr 18, 2019

mccheah commented Apr 18, 2019

vanzin commented Apr 18, 2019

mccheah commented Apr 18, 2019

vanzin commented Apr 18, 2019

mccheah commented Apr 18, 2019

vanzin commented Apr 18, 2019

squito commented Apr 19, 2019

yifeih commented Apr 19, 2019

vanzin commented Apr 19, 2019

mccheah commented Apr 19, 2019

mccheah commented Apr 19, 2019

vanzin commented Apr 19, 2019

Choose a reason for hiding this comment

yifeih commented Apr 29, 2019

yifeih commented Apr 29, 2019

vanzin commented Apr 29, 2019

mccheah commented May 1, 2019

vanzin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccheah commented May 7, 2019

Choose a reason for hiding this comment

ifilonenko Apr 12, 2019 •

edited

Loading