Skip to content

[SPARK-30030][INFRA] Use RegexChecker instead of TokenChecker to check org.apache.commons.lang. #26666

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 2 commits into from
Closed

[SPARK-30030][INFRA] Use RegexChecker instead of TokenChecker to check org.apache.commons.lang. #26666

wants to merge 2 commits into from

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Nov 25, 2019 •
edited
Loading

What changes were proposed in this pull request?

This PR replace TokenChecker with RegexChecker in scalastyle and fixes the missed instances.

Why are the changes needed?

This will remove the old comons-lang2 dependency from core module

BEFORE

$ dev/scalastyle
Scalastyle checks failed at following occurrences:
[error] /Users/dongjoon/PRS/SPARK-SerializationUtils/core/src/test/scala/org/apache/spark/util/PropertiesCloneBenchmark.scala:23:7: Use Commons Lang 3 classes (package org.apache.commons.lang3.*) instead
[error]     of Commons Lang 2 (package org.apache.commons.lang.*)
[error] Total time: 23 s, completed Nov 25, 2019 11:47:44 AM

AFTER

$ dev/scalastyle
Scalastyle checks passed.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the GitHub Action linter.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 25, 2019

cc @databricks-david-lewis and @srowen

srowen
srowen approved these changes Nov 25, 2019
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Yes, definitely. I think we cleaned this up a while ago, so this snuck back in. I thought we had a style rule about it but maybe I'm dreaming or it was disabled. It's used in the copy of the Hive thriftserver code, where we should probably leave it. So, yeah.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 25, 2019

Thanks you, @srowen . Yes. I'll try to clean up the pom and dependency like this at this time.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 25, 2019

Oh, interesting. We have a rule for that. Let me dig the reason how this escape from the checker.

  <check customId="commonslang2" level="error" class="org.scalastyle.scalariform.TokenChecker" enabled="true">
    <parameters><parameter name="regex">org\.apache\.commons\.lang\.</parameter></parameters>
    <customMessage>Use Commons Lang 3 classes (package org.apache.commons.lang3.*) instead
    of Commons Lang 2 (package org.apache.commons.lang.*)</customMessage>
  </check>

@databricks-david-lewis
Copy link
Contributor

databricks-david-lewis commented Nov 25, 2019

LGTM, sorry for letting it sneak back in! Maybe benchmark code is excluded from that check?

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 25, 2019

Thank you for review, @databricks-david-lewis .
Yes. I'm also suspecting that and checking now (if we have more instances like that).

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-29081][CORE][FOLLOWUP] Use commons-lang3 instead of commons-lang2 [SPARK-30030][INFRA] Use RegexChecker instead of TokenChecker to check regex Nov 25, 2019
dongjoon-hyun
dongjoon-hyun commented Nov 25, 2019
View reviewed changes
scalastyle-config.xml
</check>

<check customId="commonslang2" level="error" class="org.scalastyle.scalariform.TokenChecker" enabled="true">
<check customId="commonslang2" level="error" class="org.scalastyle.file.RegexChecker" enabled="true">
Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Nov 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@srowen and @databricks-david-lewis . This was the root cause.
I'm not sure if there is any change inside TokenChecker, but RegexChecker is better at detecting org.apache.commons.lang. since it's not a token.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 25, 2019

Since the GitHub Action Linter passed with the new rule and the changed code, I'll merge this PR. Thank you, @srowen and @databricks-david-lewis .

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-30030][INFRA] Use RegexChecker instead of TokenChecker to check regex [SPARK-30030][INFRA] Use RegexChecker instead of TokenChecker to check org.apache.commons.lang. Nov 25, 2019
@dongjoon-hyun dongjoon-hyun deleted the SPARK-29081-2 branch November 25, 2019 20:09
dongjoon-hyun added a commit that referenced this pull request Nov 25, 2019
@dongjoon-hyun
[SPARK-30030][INFRA] Use RegexChecker instead of TokenChecker to chec…
0dc22df 
…k `org.apache.commons.lang.`

This PR replace `TokenChecker` with `RegexChecker` in `scalastyle` and fixes the missed instances.

This will remove the old `comons-lang2` dependency from `core` module

**BEFORE**
```
$ dev/scalastyle
Scalastyle checks failed at following occurrences:
[error] /Users/dongjoon/PRS/SPARK-SerializationUtils/core/src/test/scala/org/apache/spark/util/PropertiesCloneBenchmark.scala:23:7: Use Commons Lang 3 classes (package org.apache.commons.lang3.*) instead
[error]     of Commons Lang 2 (package org.apache.commons.lang.*)
[error] Total time: 23 s, completed Nov 25, 2019 11:47:44 AM
```

**AFTER**
```
$ dev/scalastyle
Scalastyle checks passed.
```

No.

Pass the GitHub Action linter.

Closes #26666 from dongjoon-hyun/SPARK-29081-2.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 38240a7)
Signed-off-by: Dongjoon Hyun <[email protected]>
@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 25, 2019

This is backported to branch-2.4, too.

@SparkQA

This comment has been minimized.

Sign in to view
@SparkQA
Copy link

SparkQA commented Nov 25, 2019

Test build #114425 has finished for PR 26666 at commit f3b84d8.

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

HyukjinKwon
HyukjinKwon reviewed Nov 26, 2019
View reviewed changes
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM

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

+1 more reviewer

@databricks-david-lewis databricks-david-lewis databricks-david-lewis approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

PROJECT INFRA SPARK CORE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dongjoon-hyun @databricks-david-lewis @SparkQA @srowen @HyukjinKwon