[GitHub] [curator] dependabot[bot] commented on pull request #441: Bump jackson-databind from 2.10.0 to 2.12.7.1
dependabot[bot] commented on PR #441: URL: https://github.com/apache/curator/pull/441#issuecomment-1383104053 OK, I won't notify you again about this release, but will get in touch when a new version is available. If you'd rather skip all updates until the next major or minor version, let me know by commenting `@dependabot ignore this major version` or `@dependabot ignore this minor version`. If you change your mind, just re-open this PR and I'll resolve any conflicts on it. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #441: Bump jackson-databind from 2.10.0 to 2.12.7.1
tisonkun closed pull request #441: Bump jackson-databind from 2.10.0 to 2.12.7.1 URL: https://github.com/apache/curator/pull/441 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] dependabot[bot] opened a new pull request, #441: Bump jackson-databind from 2.10.0 to 2.12.7.1
dependabot[bot] opened a new pull request, #441: URL: https://github.com/apache/curator/pull/441 Bumps [jackson-databind](https://github.com/FasterXML/jackson) from 2.10.0 to 2.12.7.1. Commits See full diff in https://github.com/FasterXML/jackson/commits";>compare view [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/apache/curator/network/alerts). -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] computate opened a new pull request, #440: Upgrade jackson version from 2.10.0 to 2.14.0
computate opened a new pull request, #440: URL: https://github.com/apache/curator/pull/440 This commit upgrades the jackson-core and jackson-databind dependencies to 2.14.0. The following vulnerabilities are resolved with this commit: - [CVE-2022-42004] In FasterXML jackson-databind before 2.13.4, resource exhaustion can occur because of a lack of a check in BeanDeserializer._deserializeFromArray to prevent use of deeply nested arrays. An application is vulnerable only with certain customized choices for deserialization. - [CVE-2022-42003] In FasterXML jackson-databind before 2.14.0-rc1, resource exhaustion can occur because of a lack of a check in primitive value deserializers to avoid deep wrapper array nesting, when the UNWRAP_SINGLE_VALUE_ARRAYS feature is enabled. Additional fix version in 2.13.4.1 and 2.12.17.1 - [CVE-2020-36518] jackson-databind before 2.13.0 allows a Java StackOverflow exception and denial of service via a large depth of nested objects. - [CVE-2020-25649] A flaw was found in FasterXML Jackson Databind, where it did not have entity expansion secured properly. This flaw allows vulnerability to XML external entity (XXE) attacks. The highest threat from this vulnerability is data integrity. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
tisonkun commented on PR #421: URL: https://github.com/apache/curator/pull/421#issuecomment-1306838012 @kezhuw sorry for the late review. I'm going to merge this patch if the conflict gets resolved. Would you continue this patch? -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #392: CURATOR-603 Use Awaitility to instead of Thread sleep method in recipes module.
tisonkun commented on PR #392: URL: https://github.com/apache/curator/pull/392#issuecomment-1306836692 No activities. Closed. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #392: CURATOR-603 Use Awaitility to instead of Thread sleep method in recipes module.
tisonkun closed pull request #392: CURATOR-603 Use Awaitility to instead of Thread sleep method in recipes module. URL: https://github.com/apache/curator/pull/392 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #439: WIP. try resolve jaxb dependencies
tisonkun closed pull request #439: WIP. try resolve jaxb dependencies URL: https://github.com/apache/curator/pull/439 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] dependabot[bot] commented on pull request #438: Bump jackson-databind from 2.10.0 to 2.13.4.1
dependabot[bot] commented on PR #438: URL: https://github.com/apache/curator/pull/438#issuecomment-1288131684 OK, I won't notify you again about this release, but will get in touch when a new version is available. If you'd rather skip all updates until the next major or minor version, let me know by commenting `@dependabot ignore this major version` or `@dependabot ignore this minor version`. If you change your mind, just re-open this PR and I'll resolve any conflicts on it. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #438: Bump jackson-databind from 2.10.0 to 2.13.4.1
tisonkun commented on PR #438: URL: https://github.com/apache/curator/pull/438#issuecomment-1288131673 Perhaps manually upgrading. Closed as cannot automatically upgraded. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #438: Bump jackson-databind from 2.10.0 to 2.13.4.1
tisonkun closed pull request #438: Bump jackson-databind from 2.10.0 to 2.13.4.1 URL: https://github.com/apache/curator/pull/438 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun opened a new pull request, #439: WIP. try resolve jaxb dependencies
tisonkun opened a new pull request, #439: URL: https://github.com/apache/curator/pull/439 Signed-off-by: tison -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #435: CURATOR-654: Remove watcher after waiting on barrier.
tisonkun commented on code in PR #435:
URL: https://github.com/apache/curator/pull/435#discussion_r999343775
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/barriers/TestDistributedBarrier.java:
##
@@ -218,4 +230,48 @@ public Object call() throws Exception
client.close();
}
}
+
+@Test
+public void testIsSet() throws Exception
+{
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), new
RetryOneTime(1))) {
+client.start();
+
+final DistributedBarrier barrier = new DistributedBarrier(client,
"/barrier");
+barrier.setBarrier();
+
+assertTrue(barrier.isSet());
+}
+}
+
+@Test
+public void testIsNotSet() throws Exception
+{
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), new
RetryOneTime(1))) {
+client.start();
+
+final DistributedBarrier barrier = new DistributedBarrier(client,
"/barrier");
+barrier.setBarrier();
+barrier.removeBarrier();
+
+assertFalse(barrier.isSet());
+}
+}
+
+@Test
+public void testWatchersRemoved() throws Exception
+{
+CuratorFramework client = mock(CuratorFramework.class);
+WatcherRemoveCuratorFramework watcherRemoveClient =
mock(WatcherRemoveCuratorFramework.class);
+ExistsBuilder existsBuilder =
mock(ExistsBuilder.class);
+
+
when(client.newWatcherRemoveCuratorFramework()).thenReturn(watcherRemoveClient);
+when(watcherRemoveClient.checkExists()).thenReturn(existsBuilder);
+
when(existsBuilder.usingWatcher(any(Watcher.class))).thenReturn(existsBuilder);
+
+final DistributedBarrier barrier = new DistributedBarrier(client,
"/barrier");
+barrier.waitOnBarrier(1, TimeUnit.SECONDS);
+verify(watcherRemoveClient, atLeastOnce()).removeWatchers();
+}
Review Comment:
@kezhuw I agree that it can be a bug as I mentioned in
https://lists.apache.org/thread/0kcnklcxs0s5656c1sbh3crgdodbb0qg. You can reply
on the mailing list and file an issue.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] kezhuw commented on a diff in pull request #435: CURATOR-654: Remove watcher after waiting on barrier.
kezhuw commented on code in PR #435:
URL: https://github.com/apache/curator/pull/435#discussion_r999325439
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/barriers/TestDistributedBarrier.java:
##
@@ -218,4 +230,48 @@ public Object call() throws Exception
client.close();
}
}
+
+@Test
+public void testIsSet() throws Exception
+{
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), new
RetryOneTime(1))) {
+client.start();
+
+final DistributedBarrier barrier = new DistributedBarrier(client,
"/barrier");
+barrier.setBarrier();
+
+assertTrue(barrier.isSet());
+}
+}
+
+@Test
+public void testIsNotSet() throws Exception
+{
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), new
RetryOneTime(1))) {
+client.start();
+
+final DistributedBarrier barrier = new DistributedBarrier(client,
"/barrier");
+barrier.setBarrier();
+barrier.removeBarrier();
+
+assertFalse(barrier.isSet());
+}
+}
+
+@Test
+public void testWatchersRemoved() throws Exception
+{
+CuratorFramework client = mock(CuratorFramework.class);
+WatcherRemoveCuratorFramework watcherRemoveClient =
mock(WatcherRemoveCuratorFramework.class);
+ExistsBuilder existsBuilder =
mock(ExistsBuilder.class);
+
+
when(client.newWatcherRemoveCuratorFramework()).thenReturn(watcherRemoveClient);
+when(watcherRemoveClient.checkExists()).thenReturn(existsBuilder);
+
when(existsBuilder.usingWatcher(any(Watcher.class))).thenReturn(existsBuilder);
+
+final DistributedBarrier barrier = new DistributedBarrier(client,
"/barrier");
+barrier.waitOnBarrier(1, TimeUnit.SECONDS);
+verify(watcherRemoveClient, atLeastOnce()).removeWatchers();
+}
Review Comment:
I guess it might be particular important to test this as
`ZooKeeper.removeWatches` is cheating since
[ZOOKEEPER-1910](https://issues.apache.org/jira/browse/ZOOKEEPER-1910). See
also kezhuw/zookeeper-client-rust#2.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] dependabot[bot] opened a new pull request, #438: Bump jackson-databind from 2.10.0 to 2.13.4.1
dependabot[bot] opened a new pull request, #438: URL: https://github.com/apache/curator/pull/438 Bumps [jackson-databind](https://github.com/FasterXML/jackson) from 2.10.0 to 2.13.4.1. Commits See full diff in https://github.com/FasterXML/jackson/commits";>compare view [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/apache/curator/network/alerts). -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #398: CURATOR-653: fix potential double leader for LeaderLatch
tisonkun closed pull request #398: CURATOR-653: fix potential double leader for LeaderLatch URL: https://github.com/apache/curator/pull/398 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun merged pull request #436: [CURATOR-653] Proposed changes based on PR #398
tisonkun merged PR #436: URL: https://github.com/apache/curator/pull/436 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #436: [CURATOR-653] Proposed changes based on PR #398
tisonkun commented on PR #436: URL: https://github.com/apache/curator/pull/436#issuecomment-1282147354 Merging... -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #435: CURATOR-654: Remove watcher after waiting on barrier.
tisonkun commented on code in PR #435:
URL: https://github.com/apache/curator/pull/435#discussion_r997037672
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/barriers/TestDistributedBarrier.java:
##
@@ -218,4 +230,48 @@ public Object call() throws Exception
client.close();
}
}
+
+@Test
+public void testIsSet() throws Exception
+{
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), new
RetryOneTime(1))) {
+client.start();
+
+final DistributedBarrier barrier = new DistributedBarrier(client,
"/barrier");
+barrier.setBarrier();
+
+assertTrue(barrier.isSet());
+}
+}
+
+@Test
+public void testIsNotSet() throws Exception
+{
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), new
RetryOneTime(1))) {
+client.start();
+
+final DistributedBarrier barrier = new DistributedBarrier(client,
"/barrier");
+barrier.setBarrier();
+barrier.removeBarrier();
+
+assertFalse(barrier.isSet());
+}
+}
+
+@Test
+public void testWatchersRemoved() throws Exception
+{
+CuratorFramework client = mock(CuratorFramework.class);
+WatcherRemoveCuratorFramework watcherRemoveClient =
mock(WatcherRemoveCuratorFramework.class);
+ExistsBuilder existsBuilder =
mock(ExistsBuilder.class);
+
+
when(client.newWatcherRemoveCuratorFramework()).thenReturn(watcherRemoveClient);
+when(watcherRemoveClient.checkExists()).thenReturn(existsBuilder);
+
when(existsBuilder.usingWatcher(any(Watcher.class))).thenReturn(existsBuilder);
+
+final DistributedBarrier barrier = new DistributedBarrier(client,
"/barrier");
+barrier.waitOnBarrier(1, TimeUnit.SECONDS);
+verify(watcherRemoveClient, atLeastOnce()).removeWatchers();
+}
Review Comment:
Perhaps add watches and verify the following events triggered:
* DataWatchRemoved
* ChildWatchRemoved
* PersistentWatchRemoved
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #435: CURATOR-654: Remove watcher after waiting on barrier.
tisonkun commented on code in PR #435: URL: https://github.com/apache/curator/pull/435#discussion_r997017352 ## curator-recipes/src/main/java/org/apache/curator/framework/recipes/barriers/DistributedBarrier.java: ## @@ -92,6 +93,16 @@ public synchronized void removeBarrier() throws Exception } } +/** + * Indicates if the barrier is set or not. + * + * @return true if the barrier is set. + */ +public synchronized boolean isSet() throws Exception Review Comment: Annotated with `@VisibleForTesting`. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] hyperxpro opened a new pull request, #437: Add Support for TLS-enabled `TestingZooKeeperMain`
hyperxpro opened a new pull request, #437: URL: https://github.com/apache/curator/pull/437 When `ServerConfig#getSecureClientPortAddress` is not `null` then user has indicated the use of TLS. In this case, we will configure the Server factory to use TLS. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] XComp commented on pull request #436: [CURATOR-653] Proposed changes based on PR #398
XComp commented on PR #436:
URL: https://github.com/apache/curator/pull/436#issuecomment-1275765883
The timeout of {{TestPathChildrenCache}} in the [Java 11 CI
job](https://github.com/apache/curator/actions/runs/3219514422/jobs/5264980959#step:6:1595)
seems to be unrelated to this PR's change: It doesn't utilize `LeaderLatch` as
far as I can see. I created CURATOR-657 to cover this test instability
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on pull request #436: [CURATOR-653] Proposed changes based on PR #398
XComp commented on PR #436: URL: https://github.com/apache/curator/pull/436#issuecomment-1273457971 ``` 2022-10-10T13:50:53.7756841Z [INFO] Running org.apache.curator.framework.recipes.cache.TestPathChildrenCache 2022-10-10T15:00:44.1920989Z ##[error]The operation was canceled. ``` hm, looks like CI ran into a timeout exceeding the maximum threshold for a CI run of 120min due to `TestPathChildrenCache` :thinking: -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] XComp commented on pull request #398: CURATOR-653: fix potential double leader for LeaderLatch
XComp commented on PR #398: URL: https://github.com/apache/curator/pull/398#issuecomment-1273322972 Yes, but the biggest part of the diff is the last commit ([adaee91](https://github.com/apache/curator/pull/436/commits/adaee91289014f06b60e512b8377de7f9fe4ebd6)): I'm ok with reverting that one if you think it's too much. The changes related to the comments of my review of this PR are included in the commits excluding [adaee91](https://github.com/apache/curator/pull/436/commits/adaee91289014f06b60e512b8377de7f9fe4ebd6) in #436 . -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #398: CURATOR-653: fix potential double leader for LeaderLatch
tisonkun commented on PR #398: URL: https://github.com/apache/curator/pull/398#issuecomment-1273318540 @XComp OK. After looking into the patch I think #436 is better to proceed (you made a significant diff :)). I'll try to take a look this wek. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] XComp commented on pull request #398: CURATOR-653: fix potential double leader for LeaderLatch
XComp commented on PR #398: URL: https://github.com/apache/curator/pull/398#issuecomment-1273315990 It was a bit trickier but I managed to base the PR based on his branch (his branch didn't show up when I was doing it the way I usually create PRs ¯\_(ツ)_/¯). https://github.com/woaishixiaoxiao/curator/pull/1. Although, I'm not sure whether that's of any help because the original author would have to merge it into his branch, wouldn't he? -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #398: CURATOR-653: fix potential double leader for LeaderLatch
tisonkun commented on PR #398: URL: https://github.com/apache/curator/pull/398#issuecomment-1273296103 > but couldn't base it onto this PR Actually, you can send a pull request with base branch = https://github.com/woaishixiaoxiao/curator/tree/fixbug/LeaderLatch-double-master and head branch = your branch :) -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] XComp commented on pull request #398: CURATOR-653: fix potential double leader for LeaderLatch
XComp commented on PR #398: URL: https://github.com/apache/curator/pull/398#issuecomment-1273281727 > @XComp Thank you very much for the comments. Could you send a pull request based on the current patch? I don't know whether I can directly merge on the forked repository but at least I can submit the patch when you made it :) I created PR #436 but couldn't base it onto this PR. I handled my review comments individually to make it easier to select relevant changes. The last commit is a bigger refactoring of the test. You might want to leave that out if you think that it's too much of a change. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] XComp opened a new pull request, #436: [CURATOR-653] Proposed changes based on PR #398
XComp opened a new pull request, #436: URL: https://github.com/apache/curator/pull/436 This PR is based on PR #398 and adds a few comments from [my review of that PR](https://github.com/apache/curator/pull/398#pullrequestreview-1134363474) -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #398: CURATOR-653: fix potential double leader for LeaderLatch
tisonkun commented on PR #398: URL: https://github.com/apache/curator/pull/398#issuecomment-1272708052 @XComp Thank you very much for the comments. Could you send a pull request based on the current patch? I don't know whether I can directly merge on the forked repository but at least I can submit the patch when you made it :) -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #398: CURATOR-653: fix potential double leader for LeaderLatch
XComp commented on code in PR #398:
URL: https://github.com/apache/curator/pull/398#discussion_r990006956
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -540,10 +540,17 @@ public String getLastPathIsLeader()
@VisibleForTesting
volatile CountDownLatch debugResetWaitLatch = null;
+@VisibleForTesting
+volatile CountDownLatch debugRestWaitBeforeNodeDelete = null;
Review Comment:
```suggestion
volatile CountDownLatch debugResetWaitBeforeNodeDeleteLatch = null;
```
There's a typo in the name. Additionally, we might want to add `Latch` at
the end to reflect the purpose of this member analogously to the other latches.
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -220,6 +223,108 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void testCheckLeaderShipTiming() throws Exception
+{
+final String latchPath = "/test";
+Timing timing = new Timing();
+List latches = Lists.newArrayList();
+List clients = Lists.newArrayList();
+final BlockingQueue states = Queues.newLinkedBlockingQueue();
+ExecutorService executorService = Executors.newFixedThreadPool(2);
+for ( int i = 0; i < 2; ++i ) {
+try {
+CuratorFramework client = CuratorFrameworkFactory.builder()
+.connectString(server.getConnectString())
+.connectionTimeoutMs(1)
+.sessionTimeoutMs(6)
+.retryPolicy(new RetryOneTime(1))
+.connectionStateErrorPolicy(new
StandardConnectionStateErrorPolicy())
+.build();
+ConnectionStateListener stateListener = new
ConnectionStateListener()
+{
+@Override
+public void stateChanged(CuratorFramework client,
ConnectionState newState)
+{
+if (newState == ConnectionState.CONNECTED) {
+states.add(newState.name());
+}
+}
+};
+
client.getConnectionStateListenable().addListener(stateListener);
+client.start();
+clients.add(client);
+LeaderLatch latch = new LeaderLatch(client, latchPath,
String.valueOf(i));
+LeaderLatchListener listener = new LeaderLatchListener() {
+@Override
+public void isLeader() {
+states.add("true");
+}
+
+@Override
+public void notLeader() {
+states.add("false");
+}
+};
+latch.addListener(listener);
+latch.start();
+latches.add(latch);
+assertEquals(states.poll(timing.forWaiting().milliseconds(),
TimeUnit.MILLISECONDS), ConnectionState.CONNECTED.name());
+if (i == 0) {
+
assertEquals(states.poll(timing.forWaiting().milliseconds(),
TimeUnit.MILLISECONDS), "true");
Review Comment:
```suggestion
assertEquals("The first LeaderLatch instance should
acquire leadership.", states.poll(timing.forWaiting().milliseconds(),
TimeUnit.MILLISECONDS), "true");
```
nit: maybe adding a bit more context to this polling here to describe the
test case
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -220,6 +223,108 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void testCheckLeaderShipTiming() throws Exception
+{
+final String latchPath = "/test";
+Timing timing = new Timing();
+List latches = Lists.newArrayList();
+List clients = Lists.newArrayList();
+final BlockingQueue states = Queues.newLinkedBlockingQueue();
+ExecutorService executorService = Executors.newFixedThreadPool(2);
+for ( int i = 0; i < 2; ++i ) {
+try {
+CuratorFramework client = CuratorFrameworkFactory.builder()
+.connectString(server.getConnectString())
+.connectionTimeoutMs(1)
+.sessionTimeoutMs(6)
+.retryPolicy(new RetryOneTime(1))
+.connectionStateErrorPolicy(new
StandardConnectionStateErrorPolicy())
+.build();
+ConnectionStateListener stateListener = new
ConnectionStateListener()
+{
+@Override
+public void stateChanged(CuratorFramework client,
[GitHub] [curator] eolivelli commented on pull request #398: CURATOR-653: fix potential double leader for LeaderLatch
eolivelli commented on PR #398: URL: https://github.com/apache/curator/pull/398#issuecomment-1262293140 @tisonkun thanks for fixing the test @woaishixiaoxiao do you agree with @tisonkun 's fix ? -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #398: CURATOR-653: fix potential double leader for LeaderLatch
tisonkun commented on PR #398: URL: https://github.com/apache/curator/pull/398#issuecomment-1262277533 I adjust the test to inject force `reset`s instead of depending on connection loss. Although this means it should be a non-real-world case now, I still agree on `setLeadership(false)` on `checkLeadership` find the latch isn't the leader. `setLeadership(false)` is idempotent. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #398: CURATOR-653: fix potential double leader for LeaderLatch
tisonkun commented on PR #398: URL: https://github.com/apache/curator/pull/398#issuecomment-1258934263 Well. After #430 merged the test added in this patch failed. Need a closer look. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on PR #430: URL: https://github.com/apache/curator/pull/430#issuecomment-1258904417 Merging... I'm looking into a related change #398 and then prepare the next 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun merged pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun merged PR #430: URL: https://github.com/apache/curator/pull/430 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r980635160
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -667,9 +670,9 @@ protected void handleStateChange(ConnectionState newState)
{
try
{
-if (
client.getConnectionStateErrorPolicy().isErrorState(ConnectionState.SUSPENDED)
|| !hasLeadership.get() )
Review Comment:
... while I think we can remove the whole if condition (keep the
`getChildren` call. You can give it a try.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r980634849
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -667,9 +670,9 @@ protected void handleStateChange(ConnectionState newState)
{
try
{
-if (
client.getConnectionStateErrorPolicy().isErrorState(ConnectionState.SUSPENDED)
|| !hasLeadership.get() )
Review Comment:
I'm reverting this change because it's a OR logic but I misread it as an AND
logic in the first place.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430: URL: https://github.com/apache/curator/pull/430#discussion_r980594083 ## curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java: ## @@ -190,6 +190,12 @@ public void close() throws IOException close(closeMode); } +// for testing Review Comment: OK. cc @XComp as multiple reviewers ask for it, I'm making the change now :) -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] Randgalt commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
Randgalt commented on code in PR #430: URL: https://github.com/apache/curator/pull/430#discussion_r980218067 ## curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java: ## @@ -190,6 +190,12 @@ public void close() throws IOException close(closeMode); } +// for testing Review Comment: Use `@VisibleForTesting` -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #270: A fix for double barrier timeout behavior, and an accompanying test
tisonkun commented on PR #270: URL: https://github.com/apache/curator/pull/270#issuecomment-1257213437 Closed as inactive. Feel free to create a JIRA ticket and resubmit this patch. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #270: A fix for double barrier timeout behavior, and an accompanying test
tisonkun closed pull request #270: A fix for double barrier timeout behavior, and an accompanying test URL: https://github.com/apache/curator/pull/270 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #272: [CURATOR-473] update the comment of AdvancedTracerDriver.java
tisonkun closed pull request #272: [CURATOR-473] update the comment of AdvancedTracerDriver.java URL: https://github.com/apache/curator/pull/272 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #272: [CURATOR-473] update the comment of AdvancedTracerDriver.java
tisonkun commented on PR #272: URL: https://github.com/apache/curator/pull/272#issuecomment-1257213177 Closed as inactive. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #398: fix the bug of double leader when use LeaderLatch to select the leader
tisonkun commented on PR #398: URL: https://github.com/apache/curator/pull/398#issuecomment-1257212496 @woaishixiaoxiao can you create a JIRA ticket on https://issues.apache.org/jira/projects/CURATOR for this patch? -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #398: fix the bug of double leader when use LeaderLatch to select the leader
tisonkun commented on PR #398: URL: https://github.com/apache/curator/pull/398#issuecomment-1257212352 LGTM. I also think of this change days before. cc @eolivelli @Randgalt can you also give a review? -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #341: [CURATOR-553] Use slf4j place-holder instead String.format and concat.
tisonkun closed pull request #341: [CURATOR-553] Use slf4j place-holder instead String.format and concat. URL: https://github.com/apache/curator/pull/341 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #341: [CURATOR-553] Use slf4j place-holder instead String.format and concat.
tisonkun commented on PR #341: URL: https://github.com/apache/curator/pull/341#issuecomment-1257212126 Closed as inactive. This patch is good to continue, but cannot be merged as is. Feel free to create a new one based on the latest master. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #245: a concurrent issue found in TreeCache.class,explanation along with co…
tisonkun closed pull request #245: a concurrent issue found in TreeCache.class,explanation along with co… URL: https://github.com/apache/curator/pull/245 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #245: a concurrent issue found in TreeCache.class,explanation along with co…
tisonkun commented on PR #245: URL: https://github.com/apache/curator/pull/245#issuecomment-1257211966 Closed as inactive. Also `TreeCache` is deprecated in favor of `CuratorCache`. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #241: Shadow guava packages inside curator-framework
tisonkun closed pull request #241: Shadow guava packages inside curator-framework URL: https://github.com/apache/curator/pull/241 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #241: Shadow guava packages inside curator-framework
tisonkun commented on PR #241: URL: https://github.com/apache/curator/pull/241#issuecomment-1257211831 https://issues.apache.org/jira/browse/CURATOR-440 Closed. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #246: Detect ZooKeeper 3.5.x under OSGi
tisonkun commented on PR #246: URL: https://github.com/apache/curator/pull/246#issuecomment-1257211513 Closed as inactive. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #246: Detect ZooKeeper 3.5.x under OSGi
tisonkun closed pull request #246: Detect ZooKeeper 3.5.x under OSGi URL: https://github.com/apache/curator/pull/246 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #308: CURATOR-515: Remove use of deprecated Throwables method
tisonkun closed pull request #308: CURATOR-515: Remove use of deprecated Throwables method URL: https://github.com/apache/curator/pull/308 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r976161611
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -586,6 +608,9 @@ private void checkLeadership(List children) throws
Exception
final String localOurPath = ourPath.get();
List sortedChildren =
LockInternals.getSortedChildren(LOCK_NAME, sorter, children);
int ourIndex = (localOurPath != null) ?
sortedChildren.indexOf(ZKPaths.getNodeFromPath(localOurPath)) : -1;
+
+log.debug("checkLeadership with id: {},ourPath: {}, children: {}", id,
localOurPath, sortedChildren);
Review Comment:
```suggestion
log.debug("checkLeadership with id: {}, ourPath: {}, children: {}",
id, localOurPath, sortedChildren);
```
nit
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on PR #430: URL: https://github.com/apache/curator/pull/430#issuecomment-1253118096 @eolivelli @cammckenzie @Randgalt Perhaps we can release a 5.4.0 later this month and I'd ask for a review on this patch for a consensus whether we include it or only the debug logging part. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430: URL: https://github.com/apache/curator/pull/430#discussion_r974852907 ## curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java: ## @@ -190,6 +190,12 @@ public void close() throws IOException close(closeMode); } +// for testing Review Comment: For me, `VisibleForTesting` can mean that this method can be used in production but its visibility is wider than necessary for testing. But it's fair enough to use `VisibleForTesting`. I don't want to make the change now :) -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r974852280
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -198,9 +204,25 @@ public void close() throws IOException
* @param closeMode allows the default close mode to be overridden at the
time the latch is closed.
* @throws IOException errors
*/
-public synchronized void close(CloseMode closeMode) throws IOException
+public void close(CloseMode closeMode) throws IOException
{
-Preconditions.checkState(state.compareAndSet(State.STARTED,
State.CLOSED), "Already closed or has not been started");
+internalClose(closeMode, true);
+}
+
+private synchronized void internalClose(CloseMode closeMode, boolean
failOnClosed) throws IOException
+{
+if (!state.compareAndSet(State.STARTED, State.CLOSED))
+{
+if (failOnClosed)
+{
+throw new IllegalStateException("Already closed or has not
been started");
+}
+else
+{
+return;
+}
Review Comment:
Make sense. Although, they're almost the same. So I just keep it as is now.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r974852099
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -190,6 +190,12 @@ public void close() throws IOException
close(closeMode);
}
+// for testing
+void closeOnDemand() throws IOException
+{
+internalClose(closeMode, false);
+}
Review Comment:
I think it's a design issue we can resolve in another thread. Generally, I'd
prefer `close` to be idempotent but the original code isn't. Your test case can
fail if:
1. Checking state != CLOSE.
2. Internally closed.
3. Outside closed.
And it happens in the CI environment once.
Your suggestion can be reasonable but I don't want to spend too much time on
this bug fix PR. A package-level visible method is for internal usage :)
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] srchen1987 closed pull request #415: shutdown runSafeService
srchen1987 closed pull request #415: shutdown runSafeService URL: https://github.com/apache/curator/pull/415 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430: URL: https://github.com/apache/curator/pull/430#discussion_r974062828 ## curator-test-zk35/pom.xml: ## @@ -166,6 +166,12 @@ slf4j-log4j12 test + + +org.awaitility +awaitility +test + Review Comment: I see, sorry that I missed that :+1: -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430: URL: https://github.com/apache/curator/pull/430#discussion_r973963759 ## curator-test-zk35/pom.xml: ## @@ -166,6 +166,12 @@ slf4j-log4j12 test + + +org.awaitility +awaitility +test + Review Comment: https://github.com/apache/curator/actions/runs/3071902247/jobs/4962990393 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r973954972
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -190,6 +190,12 @@ public void close() throws IOException
close(closeMode);
}
+// for testing
+void closeOnDemand() throws IOException
+{
+internalClose(closeMode, false);
+}
Review Comment:
For now, this is a test-only method. I'm a bit reluctant to add test-related
code in production code rather than providing a utilily class instead (to be
fair: we do that in curator code in other locations like the
`debug*CountDownLatches` in the `LeaderLatch` implementation). I see a risk if
convenient methods like this are available in production: They might make it
easier to write less-consistent production code. :thinking:
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -190,6 +190,12 @@ public void close() throws IOException
close(closeMode);
}
+// for testing
Review Comment:
```suggestion
@VisibleForTesting
```
Isn't this annotation the better way of documenting this here?
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -190,6 +190,12 @@ public void close() throws IOException
close(closeMode);
}
+// for testing
+void closeOnDemand() throws IOException
+{
+internalClose(closeMode, false);
+}
Review Comment:
...but if you want to stick to it, I would vote for coming up with a more
descriptive name for it: Something like `closeIfStillRunning`
##
curator-test-zk35/pom.xml:
##
@@ -166,6 +166,12 @@
slf4j-log4j12
test
+
+
+org.awaitility
+awaitility
+test
+
Review Comment:
Why is this change necessary?
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -198,9 +204,25 @@ public void close() throws IOException
* @param closeMode allows the default close mode to be overridden at the
time the latch is closed.
* @throws IOException errors
*/
-public synchronized void close(CloseMode closeMode) throws IOException
+public void close(CloseMode closeMode) throws IOException
{
-Preconditions.checkState(state.compareAndSet(State.STARTED,
State.CLOSED), "Already closed or has not been started");
+internalClose(closeMode, true);
+}
+
+private synchronized void internalClose(CloseMode closeMode, boolean
failOnClosed) throws IOException
+{
+if (!state.compareAndSet(State.STARTED, State.CLOSED))
+{
+if (failOnClosed)
+{
+throw new IllegalStateException("Already closed or has not
been started");
+}
+else
+{
+return;
+}
Review Comment:
```suggestion
return;
```
nit: Is the `else` branch required based on code guidelines? If not, I'd
propose removing it to shorten the code. The code format (having opening
brackets on a new line) makes the code long, anyway already.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r973523533
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -586,6 +586,9 @@ private void checkLeadership(List children) throws
Exception
final String localOurPath = ourPath.get();
List sortedChildren =
LockInternals.getSortedChildren(LOCK_NAME, sorter, children);
int ourIndex = (localOurPath != null) ?
sortedChildren.indexOf(ZKPaths.getNodeFromPath(localOurPath)) : -1;
+
+log.debug("checkLeadership with ourPath: {}, children: {}",
localOurPath, sortedChildren);
Review Comment:
Updated in cc4c148a1ee3d6513f4b9f2b5fe462caee8e41fe.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r973525882
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances are joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
+
+// no extra CountDownLatch needs to be set here because
candidate #1 will rely on candidate #0
+latchCandidate1.start();
+timing.sleepABit();
Review Comment:
Resolved.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r973523533
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -586,6 +586,9 @@ private void checkLeadership(List children) throws
Exception
final String localOurPath = ourPath.get();
List sortedChildren =
LockInternals.getSortedChildren(LOCK_NAME, sorter, children);
int ourIndex = (localOurPath != null) ?
sortedChildren.indexOf(ZKPaths.getNodeFromPath(localOurPath)) : -1;
+
+log.debug("checkLeadership with ourPath: {}, children: {}",
localOurPath, sortedChildren);
Review Comment:
Updated in e6ab28b80f19ad4c86118cd99ff7a3bee6579889.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r970793303
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -667,9 +670,9 @@ protected void handleStateChange(ConnectionState newState)
{
try
{
-if (
client.getConnectionStateErrorPolicy().isErrorState(ConnectionState.SUSPENDED)
|| !hasLeadership.get() )
+if (
client.getConnectionStateErrorPolicy().isErrorState(ConnectionState.SUSPENDED) )
{
-reset();
+getChildren();
Review Comment:
Ok, after revisiting your response in [that thread earlier in this
PR](https://github.com/apache/curator/pull/430#discussion_r967852427), I notice
again that `getChildren` is the proper way of doing it:
* either the corresponding child node is still around: Then we would pick up
the leadership again.
* if it's not around, `reset()` will be called, anyway
Sorry for the confusion. ...to many threads -.-
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r970541350
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -586,6 +586,9 @@ private void checkLeadership(List children) throws
Exception
final String localOurPath = ourPath.get();
List sortedChildren =
LockInternals.getSortedChildren(LOCK_NAME, sorter, children);
int ourIndex = (localOurPath != null) ?
sortedChildren.indexOf(ZKPaths.getNodeFromPath(localOurPath)) : -1;
+
+log.debug("checkLeadership with ourPath: {}, children: {}",
localOurPath, sortedChildren);
Review Comment:
```suggestion
log.debug("[{}] checkLeadership with ourPath: {}, children: {}", id,
localOurPath, sortedChildren);
```
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances are joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
+
+// no extra CountDownLatch needs to be set here because
candidate #1 will rely on candidate #0
+latchCandidate1.start();
+timing.sleepABit();
Review Comment:
You need to add `Awaitility` to the pom of the `curator-recipes` module:
```
org.awaitility
awaitility
test
```
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances are joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
+
+// no extra CountDownLatch needs to be set here because
candidate #1 will rely on candidate #0
+latchCandidate1.start();
+timing.sleepABit();
Review Comment:
```suggestion
// we want to make sure that the leader gets leadership
before other instances are going to join the party
waitForALeader(Collections.singletonList(latchInitialLeader), new Timing());
// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r970543869
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
Review Comment:
@XComp sounds interesting. You can directly make a PR against this patch
branch and I'll review it. Sorry that I may not have too much time recently to
cooperate on this patch :)
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r970539512
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
Review Comment:
I found a better solution for that by introducing another ZK client that is
used for monitoring the children. That way, we don't end up being blocked on
concurrent calls in the actual test. I'm gonna propose the change in a separate
review.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r970539512
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
Review Comment:
I found a better solution for that by introducing another ZK client that is
used for monitoring the children. That way, we don't end up being blocked on
concurrent calls in the actual test.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r969519573
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
Review Comment:
tbh, I'm not really happy with the sleep here and in [line
248](https://github.com/apache/curator/pull/430/files#diff-75966280cab1f9788b771d244e889731ba35c7918d365c070565e070d5b801ebR248)
because they are a cause for instabilities: The `close` in [line
251](https://github.com/apache/curator/pull/430/files#diff-75966280cab1f9788b771d244e889731ba35c7918d365c070565e070d5b801ebR251)
has to happen after the child nodes for `candidate #0` and `candidate #1` are
created. AFAIU, we cannot ensure that with the sleep calls due to the
asynchronous nature of the `start` command that is triggered right before each
sleep.
I tried to add a `waitForCondition` instead, when coming up with this test,
initially, that would wait for corresponding child to be created.
Unfortunately, this resulted in the test blocking forever because (I guess) the
await on the `latchCandidate0.debugCheckLeaderShipLatch` is blocking the thread
that executes the `CuratorFrameworkImpl#backgroundOperations` queue which makes
any subsequent operation (including the check for children nodes) on the same
client being blocked as well.
I hoped that somebody else could come up with a better approach here.
:thinking:
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r969519573
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
Review Comment:
tbh, I'm not really happy with the sleep here and in [line
248](https://github.com/apache/curator/pull/430/files#diff-75966280cab1f9788b771d244e889731ba35c7918d365c070565e070d5b801ebR248)
because they are a cause for instabilities: The `close` in [line
251](https://github.com/apache/curator/pull/430/files#diff-75966280cab1f9788b771d244e889731ba35c7918d365c070565e070d5b801ebR251)
has to happen after the child nodes for `candidate #0` and `candidate #1` are
created. AFAIU, we cannot ensure that with the sleep calls due to the
asynchronous nature of the `start` command that is triggered right before each
sleep.
I tried to add a `waitForCondition` instead, when coming up with this test,
initially, that would wait for corresponding child to be created.
Unfortunately, this resulted in the test blocking forever because (I guess) the
await on the `latchCandidate0.debugCheckLeaderShipLatch` is executed in the
main thread which makes any subsequent operation (including the check for
children nodes) being blocked.
I hoped that somebody else could come up with a better approach here.
:thinking:
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r969546342
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
Review Comment:
There's a proposal to work around that issue in [the comment
below](https://github.com/apache/curator/pull/430/files#r969545913)
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r969545913
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
+
+// no extract CountDownLatch needs to be set here because
candidate #1 will rely on candidate #0
+latchCandidate1.start();
+timing.sleepABit();
Review Comment:
```suggestion
assertNull(latchCandidate0.getOurPath());
assertNull(latchCandidate1.getOurPath());
latchCandidate0.start();
latchCandidate1.start();
Awaitility.await("Wait for the candidates to have a path
assigned to them.")
.pollInterval(Duration.ofMillis(100))
.pollInSameThread()
.until(() -> latchCandidate0.getOurPath() != null &&
latchCandidate1.getOurPath() != null);
```
Would that work? The `LeaderLatch#ourPath` is set after the child is
created. WDYT?
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r969519573
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
Review Comment:
tbh, I'm not really happy with the sleep here and in [line
248](https://github.com/apache/curator/pull/430/files#diff-75966280cab1f9788b771d244e889731ba35c7918d365c070565e070d5b801ebR248)
because they are a cause for instabilities: The `close` in [line
251](https://github.com/apache/curator/pull/430/files#diff-75966280cab1f9788b771d244e889731ba35c7918d365c070565e070d5b801ebR251)
has to happen after the child nodes for `candidate #0` and `candidate #1` are
created. AFAIU, we cannot ensure that with the sleep calls due to the
asynchronous nature of the `start` command that is triggered right before each
sleep.
I tried to add a `waitForCondition` instead, when coming up with this test,
initially, that would wait for corresponding child to be created.
Unfortunately, this resulted in the test blocking forever because (I guess) the
await on the `latchCandidate0.debugCheckLeaderShipLatch` is executed in the
main thread which makes any subsequent operation (including the check for
children nodes) being blocked.
I hoped that somebody else could come up with a better approach here.
:innocent:
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r969519573
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
Review Comment:
tbh, I'm not really happy with the sleep here and in [line
248](https://github.com/apache/curator/pull/430/files#diff-75966280cab1f9788b771d244e889731ba35c7918d365c070565e070d5b801ebR248)
because they are a cause for instabilities: The `close` in [line
251](https://github.com/apache/curator/pull/430/files#diff-75966280cab1f9788b771d244e889731ba35c7918d365c070565e070d5b801ebR251)
has to happen after the child nodes for `candidate #0` and `candidate #1` are
created. AFAIU, we cannot ensure that with the sleep calls due to the
asynchronous nature of the `start` command that is triggered right before each
sleep.
I tried to add a `waitForCondition` instead, when coming up with this test
first, that would wait for the child to be created. Unfortunately, this
resulted in the test blocking forever because (I guess) the await on the
`latchCandidate0.debugCheckLeaderShipLatch` is called in the main thread which
makes any subsequent operation (including the check for children nodes) being
blocked.
I hoped that somebody else could come up with a better approach here.
:innocent:
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r969519573
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
Review Comment:
tbh, I'm not really happy with the sleep here and in [line
248](https://github.com/apache/curator/pull/430/files#diff-75966280cab1f9788b771d244e889731ba35c7918d365c070565e070d5b801ebR248)
because they are a cause for instabilities: The `close` in [line
251](https://github.com/apache/curator/pull/430/files#diff-75966280cab1f9788b771d244e889731ba35c7918d365c070565e070d5b801ebR251)
has to happen after the child nodes for `candidate #0` and `candidate #1` are
created. AFAIU, we cannot ensure that with the sleep calls due to the
asynchronous nature of the `start` command.
I tried to add a `waitForCondition` instead, when coming up with this test
first, that would wait for the child to be created. Unfortunately, this
resulted in the test blocking forever because (I guess) the await on the
`latchCandidate0.debugCheckLeaderShipLatch` is called in the main thread which
makes any subsequent operation (including the check for children nodes) being
blocked.
I hoped that somebody else could come up with a better approach here.
:innocent:
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r969507748
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
Review Comment:
```suggestion
// we want to make sure that the leader gets leadership
before other instances are joining the party
```
and another typo
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
+waitForALeader(Collections.singletonList(latchInitialLeader),
new Timing());
+
+// candidate #0 will wait for the leader to go away - this
should happen after the child nodes are retrieved by candidate #0
+latchCandidate0.debugCheckLeaderShipLatch = new
CountDownLatch(1);
+
+latchCandidate0.start();
+timing.sleepABit();
Review Comment:
tbh, I'm not really happy with the sleep here and in line 248 because they
are a cause for instabilities: The `close` in line 251 has to happen after the
child nodes for `candidate #0` and `candidate #1` are created. AFAIU, we cannot
ensure that with the sleep calls due to the asynchronous nature of the `start`
command.
I tried to add a `waitForCondition` instead, when coming up with this test
first, that would wait for the child to be created. Unfortunately, this
resulted in the test blocking forever because (I guess) the await on the
`latchCandidate0.debugCheckLeaderShipLatch` is called in the main thread which
makes any subsequent operation (including the check for children nodes) being
blocked.
I hoped that somebody else could come up with a better approach here.
:innocent:
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,56 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void
testLeadershipElectionWhenNodeDisappearsAfterChildrenAreRetrieved() throws
Exception
+{
+final String latchPath = "/foo/bar";
+final Timing2 timing = new Timing2();
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(),
timing.connection(), new RetryOneTime(1)))
+{
+client.start();
+LeaderLatch latchInitialLeader = new LeaderLatch(client,
latchPath, "initial-leader");
+LeaderLatch latchCandidate0 = new LeaderLatch(client, latchPath,
"candidate-0");
+LeaderLatch latchCandidate1 = new LeaderLatch(client, latchPath,
"candidate-1");
+
+try
+{
+latchInitialLeader.start();
+
+// we want to make sure that the leader gets leadership before
other instances joining the party
+waitForALeader(Collections.singletonList(latchInitialLe
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r967853023
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -667,9 +670,9 @@ protected void handleStateChange(ConnectionState newState)
{
try
{
-if (
client.getConnectionStateErrorPolicy().isErrorState(ConnectionState.SUSPENDED)
|| !hasLeadership.get() )
Review Comment:
If you take a look at FLINK-10052, the final solution is using a
SessionErrorPolicy that will skip this `if` block. Since a ConnectionLoss may
be only network unstable instead of the node lost its leadership (the ephemeral
node). Before this patch it's `reset` to be called and actively give up the
leadership, it will cause reelection, increase ZK workload and cause further
inconsistency.
The thorough solution should be something like I proposed in
https://github.com/apache/flink/pull/9878, but I failed to contribute it to the
upstream (FLINK-10052 takes more than 2 years to be merged. It's not a good
experience to me, lol). We run with the solution in Tencent for years and it
works well :)
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on PR #430: URL: https://github.com/apache/curator/pull/430#issuecomment-1242995908 Updated. I believe this patch is ready to merge. Please help with reviewing @eolivelli @Randgalt @cammckenzie -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch
tisonkun commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r967851944
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##
@@ -218,6 +218,30 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+@Test
+public void testOurPathDeletedOnReconnect() throws Exception
Review Comment:
Thanks! Integrated.
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -717,6 +720,7 @@ else if ( !oldValue && newValue )
private void setNode(String newValue) throws Exception
{
String oldPath = ourPath.getAndSet(newValue);
+log.debug("setNode with oldPath: {}, newValue: {}", oldPath, newValue);
Review Comment:
Updated.
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##
@@ -667,9 +670,9 @@ protected void handleStateChange(ConnectionState newState)
{
try
{
-if (
client.getConnectionStateErrorPolicy().isErrorState(ConnectionState.SUSPENDED)
|| !hasLeadership.get() )
Review Comment:
It's for backward compatibility that once we treat ConnectionLoss as
ErrorState. I agree with that since we don't give up the leadership (`reset`)
actively, we can remove the `if` condition (but still keep `getChildren`, of
course).
However, it can be a follow-up improvement that doesn't affect the
correctness. You're welcome to make the patch.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #313: Build on jdk Set release target to 8 for jdk11 compiler
tisonkun commented on PR #313: URL: https://github.com/apache/curator/pull/313#issuecomment-1238178504 We already build for JDK8 and JDK11 on master. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #313: Build on jdk Set release target to 8 for jdk11 compiler
tisonkun closed pull request #313: Build on jdk Set release target to 8 for jdk11 compiler URL: https://github.com/apache/curator/pull/313 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #243: [Fix] Curator-444
tisonkun closed pull request #243: [Fix] Curator-444 URL: https://github.com/apache/curator/pull/243 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #243: [Fix] Curator-444
tisonkun commented on PR #243: URL: https://github.com/apache/curator/pull/243#issuecomment-1238169978 Closed as stale. There're many changes since this patch was made in the first place. Feel free to resubmit it if it's still relevant. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #194: [CURATOR-379] unfixForNamespace corrupts child path when it contains namespace
tisonkun commented on PR #194: URL: https://github.com/apache/curator/pull/194#issuecomment-1238167997 Closed as stale. There're many changes since this patch was made in the first place. Feel free to resubmit it if it's still relevant. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #194: [CURATOR-379] unfixForNamespace corrupts child path when it contains namespace
tisonkun closed pull request #194: [CURATOR-379] unfixForNamespace corrupts child path when it contains namespace URL: https://github.com/apache/curator/pull/194 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #182: CURATOR-373 Add option to PersistentNode to not overwrite/delete existing node
tisonkun commented on PR #182: URL: https://github.com/apache/curator/pull/182#issuecomment-1238167670 Closed as stale. There're many changes since this patch was made in the first place. Feel free to resubmit it if it's still relevant. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #182: CURATOR-373 Add option to PersistentNode to not overwrite/delete existing node
tisonkun closed pull request #182: CURATOR-373 Add option to PersistentNode to not overwrite/delete existing node URL: https://github.com/apache/curator/pull/182 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #141: [CURATOR-314] Listenable support for GroupMember recipe
tisonkun closed pull request #141: [CURATOR-314] Listenable support for GroupMember recipe URL: https://github.com/apache/curator/pull/141 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #141: [CURATOR-314] Listenable support for GroupMember recipe
tisonkun commented on PR #141: URL: https://github.com/apache/curator/pull/141#issuecomment-1238167114 Closed as stale. There're many changes since this patch was made in the first place. Feel free to resubmit it if it's still relevant. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #112: [CURATOR-265] Make sure EnsembleTracker only notifies listeners when …
tisonkun closed pull request #112: [CURATOR-265] Make sure EnsembleTracker only notifies listeners when … URL: https://github.com/apache/curator/pull/112 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #112: [CURATOR-265] Make sure EnsembleTracker only notifies listeners when …
tisonkun commented on PR #112: URL: https://github.com/apache/curator/pull/112#issuecomment-1238161002 Closed as stale. There're many changes since this patch was made in the first place. Feel free to resubmit it if it's still relevant. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #89: CURATOR-232 Consolidate Timing and Client
tisonkun commented on PR #89: URL: https://github.com/apache/curator/pull/89#issuecomment-1238160442 Closed as stale. There're many changes since this patch was made in the first place. Feel free to resubmit it if it's still relevant. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #89: CURATOR-232 Consolidate Timing and Client
tisonkun closed pull request #89: CURATOR-232 Consolidate Timing and Client URL: https://github.com/apache/curator/pull/89 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #87: CURATOR-235 LeaderSelector.internalRequeue should be private
tisonkun closed pull request #87: CURATOR-235 LeaderSelector.internalRequeue should be private URL: https://github.com/apache/curator/pull/87 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #87: CURATOR-235 LeaderSelector.internalRequeue should be private
tisonkun commented on PR #87: URL: https://github.com/apache/curator/pull/87#issuecomment-1238157507 This is on the master https://github.com/apache/curator/commit/0bec8a066f8a4b5d26ef3621800543a54b46ac3e. But it's from another author. Sorry that we don't handle this PR in time >_< -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [curator] tisonkun closed pull request #74: LeaderSelector mutex resurrection
tisonkun closed pull request #74: LeaderSelector mutex resurrection URL: https://github.com/apache/curator/pull/74 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
