Skip to content

Conversation

@sameeragarwal
Copy link
Member

What changes were proposed in this pull request?

This patch fixes a few recently introduced java style check errors in master and release branch.

As an aside, given that java linting currently fails on machines with a clean maven cache, it'd be great to find another workaround to re-enable the java style checks as part of Spark PRB.

/cc @zsxwing @JoshRosen @srowen for any suggestions

How was this patch tested?

Manual Check

@srowen
Copy link
Member

srowen commented Jan 18, 2018

I keep forgetting why we can't turn on the checks -- was it just a time thing, because it meant a whole other pass over the code with maven?

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 19, 2018

@sameeragarwal . Can we turn on Travis CI for that purpose like AppVeyor?
I made it already in Apache Spark, but it's not enabled because there is a concern about Travis CI capacity.

If Travis CI can not handle the full traffic of Apache Spark PRs, we may run it for only Java code change PRs.

@SparkQA
Copy link

SparkQA commented Jan 19, 2018

Test build #86363 has finished for PR 20323 at commit 35bfd97.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • * getStruct() method defined in the parent class. Any call to \"get\" method in this class is a

@HyukjinKwon
Copy link
Member

If Travis CI can not handle the full traffic of Apache Spark PRs, we may run it for only Java code change PRs.

@dongjoon-hyun, do you know if Travis CI supports exclusion/inclusion of only changed files? I was under impression that Travis CI doesn't have this feature although AppVeyor has.

@sameeragarwal
Copy link
Member Author

@dongjoon-hyun while I'm not opposed to adding another builder, my concern is that it's hard to justify spending 30+ mins for a simple java style check for every pull request (i.e., if we run an additional mvn install for all sbt builds).

If we think that the cost is justified, then either adding a parallel travis builder or a new amplab jenkins builder is essentially serving the same purpose from a user's perspective and we should pick whichever option makes the most sense. However, either way, I'm curious if there are cheaper alternatives to fundamentally get around this dependency problem.

@sameeragarwal
Copy link
Member Author

merging this to master/2.3. Thanks!

asfgit pushed a commit that referenced this pull request Jan 19, 2018
## What changes were proposed in this pull request?

This patch fixes a few recently introduced java style check errors in master and release branch.

As an aside, given that [java linting currently fails](#10763
) on machines with a clean maven cache, it'd be great to find another workaround to [re-enable the java style checks](https://github.com/apache/spark/blob/3a07eff5af601511e97a05e6fea0e3d48f74c4f0/dev/run-tests.py#L577) as part of Spark PRB.

/cc zsxwing JoshRosen srowen for any suggestions

## How was this patch tested?

Manual Check

Author: Sameer Agarwal <[email protected]>

Closes #20323 from sameeragarwal/java.

(cherry picked from commit 9c4b998)
Signed-off-by: Sameer Agarwal <[email protected]>
@asfgit asfgit closed this in 9c4b998 Jan 19, 2018
@dongjoon-hyun
Copy link
Member

@HyukjinKwon . Travis CI will trigger at every commit. What I mean is our script can check the Java changes only.

@sameeragarwal . Travis CI is independently running on Travis CI site like AppVoyer. That is the exact reason why I added Travis CI.

  • Travis CI will finish faster than Jenkins.
  • Travis CI will not add a time or any overload to Jenkins.

Please see this.

@dongjoon-hyun
Copy link
Member

I'm wondering why we are okay for AppVoyer and not okay for Travis CI. :)

@srowen
Copy link
Member

srowen commented Jan 19, 2018

The only downside is spreading the CI here across many different systems. I know we add Appveyor because it was the only way to test on Windows (right?). Adding Travis too just for Java style checks is more questionable. Yes it has nothing to do with Jenkins though. I think we've just punted on this and accepted that Java style checks need to be executed manually once in a while.

One middle-ground is to enable style checks in the Jenkins jobs besides the PR builder. You still don't catch violations at the time a PR is submitted, but at least catch them automatically, promptly.

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