Skip to content

Conversation

@pan3793
Copy link
Member

@pan3793 pan3793 commented Jan 29, 2022

Why are the changes needed?

The previous PR did not handle log4j 1.2 deps of spark-hive, and the current detect deps change workflow has the dirty cache issue, this PR also fix it.

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@pan3793 pan3793 requested a review from turboFei January 29, 2022 14:15
@turboFei
Copy link
Member

Warning:  [Warn] /home/runner/work/incubator-kyuubi/incubator-kyuubi/kyuubi-hive-beeline/src/main/java/org/apache/hive/beeline/KyuubiCommands.java:483: Character(char) in java.lang.Character has been deprecated
Warning:  Scalastyle:check is skipped as scalastyle.skip=true
Error: ] : Symbol 'type org.apache.log4j.AppenderSkeleton' is missing from the classpath.

@turboFei
Copy link
Member

Warning:  [Warn] /home/runner/work/incubator-kyuubi/incubator-kyuubi/kyuubi-hive-beeline/src/main/java/org/apache/hive/beeline/KyuubiCommands.java:483: Character(char) in java.lang.Character has been deprecated
Warning:  Scalastyle:check is skipped as scalastyle.skip=true
Error: ] : Symbol 'type org.apache.log4j.AppenderSkeleton' is missing from the classpath.

let me try to revert the change for kyuubi-hive-beeline module

@turboFei
Copy link
Member

you can rebase on #1854 if it is merged.

@turboFei
Copy link
Member

turboFei commented Feb 1, 2022

Dependency List Changed Detected: 
78a79 > log4j/1.2.17//log4j-1.2.17.jar 96a98 > slf4j-log4j12/1.7.30//slf4j-log4j12-1.7.30.jar
> log4j/1.2.17//log4j-1.2.17.jar
96a98
> slf4j-log4j12/1.7.30//slf4j-log4j12-1.7.30.jar ]]
To update the dependency file, run './build/dependency.sh --replace'.
+ echo 'Dependency List Changed Detected: '
+ echo 78a79 '>' log4j/1.2.17//log4j-1.2.17.jar 96a98 '>' slf4j-log4j12/1.7.30//slf4j-log4j12-1.7.30.jar
+ echo 'To update the dependency file, run '\''./build/dependency.sh --replace'\''.'
+ exit 1

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2022

Codecov Report

Merging #1856 (301acb6) into master (ec719b8) will not change coverage.
The diff coverage is n/a.

❗ Current head 301acb6 differs from pull request most recent head 2785acf. Consider uploading reports for the commit 2785acf to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1856   +/-   ##
=========================================
  Coverage     59.70%   59.70%           
  Complexity      268      268           
=========================================
  Files           282      282           
  Lines         13978    13978           
  Branches       1781     1781           
=========================================
  Hits           8346     8346           
  Misses         4924     4924           
  Partials        708      708           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec719b8...2785acf. Read the comment docs.

@pan3793 pan3793 changed the title [KYUUBI #1769][FOLLOWUP] Exclude log4j 12 deps from test deps [KYUUBI #1769][FOLLOWUP] Exclude log4j 12 deps from spark-hive and fix detect deps change workflow Feb 1, 2022
@pan3793 pan3793 self-assigned this Feb 1, 2022
@pan3793 pan3793 added this to the v1.5.0 milestone Feb 1, 2022
@pan3793
Copy link
Member Author

pan3793 commented Feb 1, 2022

@turboFei The detect dependencyList changes workflow failed because of cache issue, I updated and solved it, please take a look again.

Copy link
Member

@turboFei turboFei left a comment

Choose a reason for hiding this comment

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

LGTM

@turboFei turboFei closed this in b2bad04 Feb 1, 2022
@turboFei
Copy link
Member

turboFei commented Feb 1, 2022

thanks, merged to master

@pan3793 pan3793 deleted the log4j2 branch February 1, 2022 16:06
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.

3 participants