Skip to content

Conversation

@NihalJain
Copy link
Contributor

@NihalJain NihalJain commented Aug 17, 2024

  • Remove EOL'ed gmaven-plugin which is unnecessarily too complex and does not work with JDK17
  • Refactor flags for UT and IT for ease of profile based run
  • Copy JDK17 flags from HBase and Spark
  • Add profile for JDK11 and JDK17 build

@NihalJain NihalJain marked this pull request as draft August 17, 2024 09:44
@NihalJain
Copy link
Contributor Author

Marking as draft, will self review first and make it non-draft later!

@NihalJain NihalJain force-pushed the HBASE-28382 branch 2 times, most recently from 657573e to 35f6a8b Compare August 17, 2024 11:18
@NihalJain NihalJain marked this pull request as ready for review August 17, 2024 11:21
@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 15s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
-0 ⚠️ test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ master Compile Tests _
+0 🆗 mvndep 0m 27s Maven dependency ordering for branch
+1 💚 mvninstall 1m 30s master passed
+1 💚 compile 0m 52s master passed
+1 💚 spotless 0m 15s branch has no errors when running spotless:check.
+1 💚 javadoc 0m 51s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 7s Maven dependency ordering for patch
+1 💚 mvninstall 0m 49s the patch passed
+1 💚 compile 0m 51s the patch passed
+1 💚 javac 0m 51s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 xml 0m 3s The patch has no ill-formed XML file.
+1 💚 spotless 0m 12s patch has no errors when running spotless:check.
+1 💚 javadoc 0m 49s the patch passed
_ Other Tests _
+1 💚 unit 8m 20s root in the patch passed.
17m 17s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-132/1/artifact/yetus-precommit-check/output/Dockerfile
GITHUB PR #132
Optional Tests dupname javac javadoc unit spotless xml compile
uname Linux 7c93e945b9cd 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 GNU/Linux
Build tool hb_maven
Personality dev-support/jenkins/hbase-personality.sh
git revision master / 1498a91
Default Java Oracle Corporation-1.8.0_282-b08
Test Results https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-132/1/testReport/
Max. process+thread count 942 (vs. ulimit of 12500)
modules C: spark spark/hbase-spark spark/hbase-spark-it . U: .
Console output https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-132/1/console
versions git=2.20.1
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@NihalJain
Copy link
Contributor Author

Hi @Reidddddd would you be able to review this one?

@ndimiduk
Copy link
Member

Can you also update the Jenkinsfile with additional stages so that we test builds on all the supported JVM major versions?

@ndimiduk
Copy link
Member

That is, I assume that this commit does not also deprecate the JDK11 (and JDK8?) support.

@NihalJain
Copy link
Contributor Author

Hi @ndimiduk thank you for having a look at this one.

Can you also update the Jenkinsfile with additional stages so that we test builds on all the supported JVM major versions?

Sure I had created a sub task to handle the build part: HBASE-28789. But I could update the current PR as well to take care of same. Let me know how I should proceed.

That is, I assume that this commit does not also deprecate the JDK11 (and JDK8?) support.

Yes it does not deprecate JDK8 support.

@ndimiduk
Copy link
Member

Ah okay. Sorry, I only looked at the PR, not Jira.

Yes, I think it would be nice if it all happened together because then the new pre-commit checker will verify the code before it lands in the repo. Only if one or the other change ends up being too big for reviews to be practical would I separate them.

@NihalJain
Copy link
Contributor Author

Yes, I think it would be nice if it all happened together because then the new pre-commit checker will verify the code before it lands in the repo.

Sure let me work and update this PR with Jenkins related changes as well.

@stoty
Copy link
Contributor

stoty commented May 15, 2025

Care to rebase this @NihalJain ?

@NihalJain
Copy link
Contributor Author

Care to rebase this @NihalJain ?

Sure @stoty

-verbose:gc -XX:+PrintCommandLineFlags -XX:+PrintFlagsFinal -XX:+IgnoreUnrecognizedVMOptions</spark-it.args>
<spark-it.argLine>${argLine} ${spark-it.args}</spark-it.argLine>
<hbase-surefire.argLine>-Djava.security.manager=allow</hbase-surefire.argLine>
<hbase-surefire.jdk11.flags>-Dorg.apache.hbase.thirdparty.io.netty.tryReflectionSetAccessible=true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we need jdk11 any more? i think should drop this altogether and be only on jdk17?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just double checked, branch-2.6 still has jdk11

@Apache-HBase

This comment was marked as outdated.

@Apache-HBase

This comment was marked as outdated.

…'ed gmaven-plugin which is unnecessarily too complex and does not work with JDK17 - Refactor flags for UT and IT for ease of profile based run - Bump and replace mockito-all with mockito-bom and update dependencies as needed - Copy JDK17 flags from HBase and Spark - Add profile for JDK11 and JDK17 build
@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 31s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
-0 ⚠️ test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ master Compile Tests _
+0 🆗 mvndep 0m 31s Maven dependency ordering for branch
+1 💚 mvninstall 1m 39s master passed
+1 💚 compile 1m 1s master passed
+1 💚 spotless 0m 16s branch has no errors when running spotless:check.
+1 💚 javadoc 0m 57s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 7s Maven dependency ordering for patch
+1 💚 mvninstall 0m 51s the patch passed
+1 💚 compile 0m 55s the patch passed
+1 💚 javac 0m 55s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 xml 0m 2s The patch has no ill-formed XML file.
+1 💚 spotless 0m 11s patch has no errors when running spotless:check.
+1 💚 javadoc 0m 53s the patch passed
_ Other Tests _
+1 💚 unit 9m 2s root in the patch passed.
17m 59s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-132/4/artifact/yetus-precommit-check/output/Dockerfile
GITHUB PR #132
Optional Tests dupname javac javadoc unit spotless xml compile
uname Linux aa03f8ba036e 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 GNU/Linux
Build tool hb_maven
Personality dev-support/jenkins/hbase-personality.sh
git revision master / 800161e
Default Java Oracle Corporation-1.8.0_282-b08
Test Results https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-132/4/testReport/
Max. process+thread count 972 (vs. ulimit of 12500)
modules C: spark spark/hbase-spark spark/hbase-spark-it . U: .
Console output https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-132/4/console
versions git=2.20.1
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@NihalJain
Copy link
Contributor Author

NihalJain commented May 15, 2025

Sorry for the noise. I messed up by rebasing and pushing an older version of this PR in another local fork of mine.

I have run build with JDK 17 in local. Works fine.
Screenshot 2025-05-15 at 11 15 54 PM

For Nick's comment I would prefer to do this as another commit (of course if this is fine for everyone). Also maybe it's time we try to move to Github actions for build taking hbase-connectors project as PoC.

@NihalJain NihalJain requested a review from stoty May 15, 2025 17:47
Copy link
Contributor

@stoty stoty left a comment

Choose a reason for hiding this comment

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

+1 LGTM

@NihalJain NihalJain merged commit ac5d120 into apache:master May 16, 2025
1 check passed
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.

4 participants