Skip to content
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

ZOOKEEPER-4726: Upgrade snappy-java version to v1.1.10.3 #2038

Closed
wants to merge 1 commit into from
Closed

ZOOKEEPER-4726: Upgrade snappy-java version to v1.1.10.3 #2038

wants to merge 1 commit into from

Conversation

vibhutisawant
Copy link
Contributor

@vibhutisawant vibhutisawant commented Jul 21, 2023 •
edited
Loading

This PR fixes the s390x builds on Apache Zookeeper Jenkins CI.

changes done are as follows:

  • Updated Snappy-java version to 1.1.10.3 as Snappy-java v1.1.10.1 is broken for s390x arch and has been fixed in v1.1.10.3.

Could any of the developers please review these changes? Thanks

@sonatype-lift
Copy link

sonatype-lift bot commented Jul 21, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

@vibhutisawant vibhutisawant changed the title Upgrade snappy-java version to v1.1.10.2 Upgrade snappy-java version to v1.1.10.3 Jul 24, 2023
@vibhutisawant
Copy link
Contributor Author

Hi @eolivelli , @anmolnar,
Could you please review this PR when you have a chance? Thank you very much!

@vibhutisawant vibhutisawant changed the title Upgrade snappy-java version to v1.1.10.3 ZOOKEEPER-4726: Upgrade snappy-java version to v1.1.10.3 Jul 27, 2023
@vibhutisawant
Copy link
Contributor Author

Hi @eolivelli , @anmolnar, Could you please take a look? Thanks.

ctubbsii
ctubbsii suggested changes Aug 24, 2023
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

In looking at the ClientSSLTest, it looks like there's a bunch of problems with it. It seems to have a mix of JUnit4 and JUnit5 code in it.

It also seems to still be using hamcrest matchers with the assertThrows from JUnit4, which are not needed. In fact, there shouldn't be any hamcrest or JUnit4 stuff in any JUnit5 tests. This test is also doing some weird things with intercepting a thrown AssertionError, which really shouldn't be done... those should be handled by the JUnit framework instead. A regular exception should be thrown from the test method, or the assertions should be checked right away, rather than rethrown and checked outside.

Overall, this test class is very weirdly written and could use some improvements. But the portion that pertains to this PR is that it should make use of Assumptions to ignore (with a message) when running on s390x.

Jenkinsfile-s390x Outdated
@@ -48,7 +48,7 @@ pipeline {
stage('BuildAndTest') {
steps {
sh "git clean -fxd"
sh "mvn verify spotbugs:check checkstyle:check -Pfull-build -Dsurefire-forkcount=4"
sh "mvn verify spotbugs:check checkstyle:check -Pfull-build -Dsurefire-forkcount=4 -Dtest=\!ClientSSLTest -DfailIfNoTests=false"
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this a bit more, I think you actually shouldn't skip the test here. Instead, do it in JUnit. In the ClientSSLTest, you can add:

    Assumptions.assumeFalse(System.getProperty("os.arch").contains("s390x"), "Test cannot run for s390x because ...").

If you add something like that to the top of a test case, JUnit will not only happily skip the test, but it will also helpfully print out an explanation of why the test was skipped. Keeping the exclusion close to the test case itself is better than writing the exclusion into the Jenkinsfile.

Also, it's not even clear that this is a good pattern to use... as it will try to execute any utility classes as test classes, in addition to the default pattern of *Test,Test*.

I think this should be changed to use the JUnit Assumption instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ctubbsii for the detailed review. Yes, I agree that skipping the test case in ClientSSLTest.java would be better than skipping it in Jenkinsfile. I will test and push the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ctubbsii, could you please take a look at the latest changes?

@vibhutisawant vibhutisawant force-pushed the s390x-ci-fix branch 2 times, most recently from 8efd19f to 14a96af Compare August 31, 2023 11:21
eolivelli
eolivelli requested changes Aug 31, 2023
View reviewed changes
zookeeper-server/src/test/java/org/apache/zookeeper/test/ClientSSLTest.java Outdated
@@ -147,6 +148,8 @@ public void testClientServerSSLWithCnxnClassName() throws Exception {
@ParameterizedTest(name = "sslProvider={0}, fipsEnabled={1}, hostnameVerification={2}")
@MethodSource("positiveTestData")
public void testClientServerSSL_positive(SslProvider sslProvider, String fipsEnabled, String hostnameVerification) throws Exception {
//Skipping this test for s390x arch as netty-tc-native is not supported
assumeFalse(System.getProperty("os.arch").contains("s390x"), " Skipping for s390x arch as netty-tcnative is not yet supported.");
Copy link
Contributor

Choose a reason for hiding this comment

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

this change is not related to this patch
can you move it to another patch please ?

Copy link
Contributor Author

@vibhutisawant vibhutisawant Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sure @eolivelli, I will create another PR to handle this change. Should I create a separate JIRA issue as well? Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it is required for each PRs, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @eolivelli, I have raised a separate PR to address the above file changes. #2057. Could you please have a look? Thank you.

@vibhutisawant
Copy link
Contributor Author

Hi @eolivelli , @ctubbsii Could you please take a look . Thank you.

@vibhutisawant
Copy link
Contributor Author

Hi @ctubbsii, could this PR be merged? Thanks.

@ctubbsii
Copy link
Member

Hi @ctubbsii, could this PR be merged? Thanks.

I'm not a committer on this project so a committer will have to merge it when they approve it. You could try to get committers to review it by sending an email to the developer mailing list.

Also, I think there might be an even newer version of snappy now. So you could update this pull request with the latest version while you're waiting for a committer to approve and merge it

@lhotari
Copy link
Member

lhotari commented Oct 2, 2023

The most recent version of snappy-java is 1.1.10.5 . I have created https://issues.apache.org/jira/browse/ZOOKEEPER-4751 / #2072 for that.

@lhotari
Copy link
Member

lhotari commented Oct 3, 2023

This PR is now obsolete since #2072 has been merged to master.

@vibhutisawant
Copy link
Contributor Author

Thanks. Closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolivelli eolivelli eolivelli requested changes

@shoothzj shoothzj shoothzj approved these changes

@ctubbsii ctubbsii ctubbsii approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vibhutisawant @ctubbsii @lhotari @eolivelli @shoothzj