You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by aajisaka <gi...@git.apache.org> on 2018/10/24 10:04:38 UTC
[GitHub] zookeeper pull request #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator Te...
GitHub user aajisaka opened a pull request:
https://github.com/apache/zookeeper/pull/676
ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQuorumPeerMain.
Detail: https://issues.apache.org/jira/browse/ZOOKEEPER-3181
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aajisaka/zookeeper ZOOKEEPER-3181
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/676.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #676
----
commit 1f16926265a849ce7a6d8b43f9d16d4c3048f619
Author: Akira Ajisaka <aa...@...>
Date: 2018-10-24T10:00:19Z
ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQuorumPeerMain.
----
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/676
-1
Wait for confirming why we have to do this.
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/676
>> I'm not familiar with Apache Curator project, so I'm not sure how actually hard it is.
@lvfangmin is a Curator committer and could probably help here to push out a new release.
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by aajisaka <gi...@git.apache.org>.
Github user aajisaka commented on the issue:
https://github.com/apache/zookeeper/pull/676
Per https://issues.apache.org/jira/browse/CURATOR-409?focusedCommentId=16661755&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16661755, this issue is not fixed in Curator 2.x and the users who are using ZK 3.4.x still rely on Curator 2.x for tests even if they upgrade Curator to 4.x.
Since there are no releases in curator 2.x for more than one year, I'm not sure CURATOR-409 will be fixed in 2.x and the maintenance release will be released soon. Therefore I'd like to this to be fixed in ZooKeeper as well, especially for ZK 3.4.x.
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/676
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2504/
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/676
retest this please
---
[GitHub] zookeeper pull request #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator Te...
Posted by aajisaka <gi...@git.apache.org>.
Github user aajisaka closed the pull request at:
https://github.com/apache/zookeeper/pull/676
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/676
>> Since there are no releases in curator 2.x for more than one year, I'm not sure CURATOR-409 will be fixed in 2.x and the maintenance release will be released soon.
How hard is it to do a back port and make a new 2.x release? This seems the most reasonable solution to me for this specific problem given Curator already did the fix for 4.x, and this approach has added benefit of patching existing ZK releases without requiring a new ZK release (we kind of very slow on release zookeeper, if not slower than Curator).
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/676
@lvfangmin
I guess the problem is not a 'test' in Curator but that class. Curator is used very much in tests of downstream projects in order to start Zookeeper servers as it provides easy ways to start single server and multi server Zookeeper systems for unit tests
That change broke Curator. Restoring the 'compatibility' is easy and it can help other projects
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by aajisaka <gi...@git.apache.org>.
Github user aajisaka commented on the issue:
https://github.com/apache/zookeeper/pull/676
Umm. All the failed tests passed on my local environment.
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by aajisaka <gi...@git.apache.org>.
Github user aajisaka commented on the issue:
https://github.com/apache/zookeeper/pull/676
@anmolnar Here is the `getQuorumPeer()` method in Apache Curator 2.12.0 https://github.com/apache/curator/blob/apache-curator-2.12.0/curator-test/src/main/java/org/apache/curator/test/TestingQuorumPeerMain.java#L56
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/676
@Randgalt I'm not familiar with the Curator release here, do you think it's easy to make a new release for 2.x for this?
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/676
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2489/
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by aajisaka <gi...@git.apache.org>.
Github user aajisaka commented on the issue:
https://github.com/apache/zookeeper/pull/676
Apache Curator 2.x release is likely to happen, so I close this PR.
Thank you for your discussion.
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by aajisaka <gi...@git.apache.org>.
Github user aajisaka commented on the issue:
https://github.com/apache/zookeeper/pull/676
@hanm It seems hard to make a new 2.x release. Patching itself is easy, however, there is no branch for developing 2.x and no activity to release 2.x. Please see https://issues.apache.org/jira/browse/CURATOR-409 for the detail.
(I'm not familiar with Apache Curator project, so I'm not sure how actually hard it is.)
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/676
@aajisaka I'm a little bit confused with this patch, because I cannot find the class `TestingQuorumPeerMain` which was overwritten the `getQuorumPeer()` method as mentioned in the Jira. Where is that class exactly?
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/676
@lvfangmin IMHO initQuorumPeer is a better name than getQuorumPeer because getQuorumPeer seems a 'getter' but is it not.
It is cleaner to have a initQuorumPeer or makeQuorumPeer.
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/676
@eolivelli I agree the naming is a bit confusing here, in this case we should rename the Jira and PR to make it more explicitly, we can change the name here, but it doesn't make sense to rename it because Curator test is failing.
@aajisaka 3.4.x is only accepting bug fixes, I'm not sure renaming for making the Curator test happy is allowed there. @anmolnar what's your opinion on this?
---
[GitHub] zookeeper issue #676: ZOOKEEPER-3181: ZOOKEEPER-2355 broke Curator TestingQu...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/676
@aajisaka we shouldn't change the code because of a failed test, and this is only for a test case which has overriding this function name in Curator test.
Isn't the right thing is only changing it in the Curator, as what you've donee in https://github.com/apache/curator/pull/219.
---