You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by GitBox <gi...@apache.org> on 2022/02/25 13:20:38 UTC
[GitHub] [curator] martin-g opened a new pull request #409: CURATOR-633: Fix backward compatibility in tests
martin-g opened a new pull request #409:
URL: https://github.com/apache/curator/pull/409
Run TestConnectionStateManager#testConnectionStateRecoversFromUnexpectedExpiredConnection only for Zookeeper 3.6.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] martin-g commented on a change in pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #409:
URL: https://github.com/apache/curator/pull/409#discussion_r815353470
##########
File path: curator-framework/src/test/java/org/apache/curator/framework/state/TestConnectionStateManager.java
##########
@@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
@Test
- public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
+ void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
Review comment:
> 1. If you remove `@Test` above, this test doesn't actually run in any condition.
I am not sure why do you explain this to me. I didn't touch `@Test` and the test is still executed in `curator-framework` Maven module, where Zookeeper is 3.6.3.
The problem is when this test is executed in `curator-test-zk35` module (Zookeeper 3.5.7) where `#queueEvent()` is not available.
https://issues.apache.org/jira/browse/ZOOKEEPER-3269 is `Fixed" only for `3.6.0`+
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] eolivelli commented on a change in pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #409:
URL: https://github.com/apache/curator/pull/409#discussion_r815357079
##########
File path: curator-framework/src/test/java/org/apache/curator/framework/state/TestConnectionStateManager.java
##########
@@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
@Test
- public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
+ void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
Review comment:
BTW if we don't have much time we can keep this patch an unblock ci and the upcoming release
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] eolivelli closed pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #409:
URL: https://github.com/apache/curator/pull/409
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] tisonkun commented on a change in pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
tisonkun commented on a change in pull request #409:
URL: https://github.com/apache/curator/pull/409#discussion_r815376664
##########
File path: curator-framework/src/test/java/org/apache/curator/framework/state/TestConnectionStateManager.java
##########
@@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
@Test
- public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
+ void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
Review comment:
> to create a set of tests that run only against zk 3.6+
@eolivelli we have already implemented this. If you want to merge a workaround for unblocking release, it's OK to me, and I'll prepare a followup for explaining.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] tisonkun commented on a change in pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
tisonkun commented on a change in pull request #409:
URL: https://github.com/apache/curator/pull/409#discussion_r815245892
##########
File path: curator-framework/src/test/java/org/apache/curator/framework/state/TestConnectionStateManager.java
##########
@@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
@Test
- public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
+ void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
Review comment:
Another perspective is that whether or not we won't to test that it's zk35compatible. If we don't intend to do that, we can move the test to another (new) class and assign it to zk36 group.
##########
File path: curator-framework/src/test/java/org/apache/curator/framework/state/TestConnectionStateManager.java
##########
@@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
@Test
- public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
+ void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
Review comment:
Another perspective is that whether or not we want to test that it's zk35compatible. If we don't intend to do that, we can move the test to another (new) class and assign it to zk36 group.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] tisonkun commented on a change in pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
tisonkun commented on a change in pull request #409:
URL: https://github.com/apache/curator/pull/409#discussion_r815245304
##########
File path: curator-framework/src/test/java/org/apache/curator/framework/state/TestConnectionStateManager.java
##########
@@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
@Test
- public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
+ void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
Review comment:
I think it's not logically incompatible.
1. If you remove `@Test` above, this test doesn't actually run in any condition.
2. The original error report on JIRA looks because https://github.com/apache/zookeeper/pull/799 wasn't include in zk 3.5. And the usage of `queueEvent` here can be emulated by bringing back bridges of `queueEvent` like https://github.com/apache/curator/pull/344/ removed - it's not a logically incompatible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] martin-g commented on a change in pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #409:
URL: https://github.com/apache/curator/pull/409#discussion_r815353470
##########
File path: curator-framework/src/test/java/org/apache/curator/framework/state/TestConnectionStateManager.java
##########
@@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
@Test
- public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
+ void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
Review comment:
> 1. If you remove `@Test` above, this test doesn't actually run in any condition.
I am not sure why do you explain this to me. I didn't touch `@Test` and the test is still executed in `curator-framework` Maven module, where Zookeeper is 3.6.3.
The problem is when this test is executed in `curator-test-zk35` module (Zookeeper 3.5.7) where `#queueEvent()` is not available.
https://issues.apache.org/jira/browse/ZOOKEEPER-3269 is "Fixed" only for `3.6.0`+
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] eolivelli commented on pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #409:
URL: https://github.com/apache/curator/pull/409#issuecomment-1051119785
Can you rebase in order to pick your changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] eolivelli commented on a change in pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #409:
URL: https://github.com/apache/curator/pull/409#discussion_r815356978
##########
File path: curator-framework/src/test/java/org/apache/curator/framework/state/TestConnectionStateManager.java
##########
@@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
@Test
- public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
+ void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
Review comment:
Maybe @tisonkun you are suggesting to create a set of tests that run only against zk 3.6+
That is to generalise the approach of this fix.
Probably we can create a annotation a little junit extension.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] martin-g commented on a change in pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #409:
URL: https://github.com/apache/curator/pull/409#discussion_r814820951
##########
File path: curator-framework/src/test/java/org/apache/curator/framework/state/TestConnectionStateManager.java
##########
@@ -92,6 +94,7 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
@Test
public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
+ assumeTrue(() -> (getMajor() == 3 && getMinor() >= 6) || (getMajor() > 4), "Zookeeper version must be 3.6 or higher");
Review comment:
This does not really work...
It seems javac inlines the values of the constants and the test still prints `3` and `6` even for `curator-test-zk35` modole.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] tisonkun commented on a change in pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
tisonkun commented on a change in pull request #409:
URL: https://github.com/apache/curator/pull/409#discussion_r815376906
##########
File path: curator-framework/src/test/java/org/apache/curator/framework/state/TestConnectionStateManager.java
##########
@@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
@Test
- public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
+ void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
+ final int major = getVersionPart("MAJOR");
+ final int minor = getVersionPart("MINOR");
+ assumeTrue(major == 3 && minor >= 6 || major > 4, "Zookeeper version must be 3.6 or higher");
Review comment:
Also, the boolean condition should not be `==3 || > 4`, what if `==4`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] tisonkun commented on a change in pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
tisonkun commented on a change in pull request #409:
URL: https://github.com/apache/curator/pull/409#discussion_r815381196
##########
File path: curator-framework/src/test/java/org/apache/curator/framework/state/TestConnectionStateManager.java
##########
@@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
@Test
- public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
+ void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
Review comment:
FYI https://github.com/apache/curator/pull/411.
@martin-g you can integrate in this PR and close #411.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] martin-g commented on pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #409:
URL: https://github.com/apache/curator/pull/409#issuecomment-1051062197
Yes!
The build fails with JDK 1.8 and 11.
On Fri, Feb 25, 2022, 18:30 Enrico Olivelli ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
>
> So you mean that this tests didn't work with 3.5?
>
> I can't remember about this problem
>
> @Randgalt <https://github.com/Randgalt> @tisonkun
> <https://github.com/tisonkun>
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/curator/pull/409#pullrequestreview-893905752>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABYUQS5Y6RDA5FJOD4GPW3U46VBVANCNFSM5PKIMVXA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] martin-g commented on pull request #409: CURATOR-633: Fix backward compatibility in tests
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #409:
URL: https://github.com/apache/curator/pull/409#issuecomment-1051312996
CI is green! 🥳
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org