You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/08/19 06:01:55 UTC
[GitHub] [helix] manick02 opened a new pull request #1292: Remove closed ZKConnectionManager from connection manager pool
manick02 opened a new pull request #1292:
URL: https://github.com/apache/helix/pull/1292
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1126
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
(Write a concise description including what, why, how)
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- [ ] The following is the result of the "mvn test" command on the appropriate module:
[INFO] Tests run: 44, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 380.898 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 44, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 06:29 min
[INFO] Finished at: 2020-08-19T11:25:45+05:30
[INFO] Final Memory: 22M/263M
[INFO] ------------------------------------------------------------------------
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1292:
URL: https://github.com/apache/helix/pull/1292#issuecomment-676695314
@narendly, @pkuwm focusing on hunting the flaky test in helix-core at this moment. Let me have a look at this one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] manick02 commented on a change in pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
manick02 commented on a change in pull request #1292:
URL: https://github.com/apache/helix/pull/1292#discussion_r483518874
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/ZkConnectionManager.java
##########
@@ -132,6 +131,10 @@ protected synchronized void close(boolean skipIfWatched) {
LOG.info("ZkConnection {} was closed.", _monitorKey);
}
+ protected boolean hasSharedWatchers() {
Review comment:
Thanks will take care as you mentioned in the below comment
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
narendly commented on pull request #1292:
URL: https://github.com/apache/helix/pull/1292#issuecomment-676635566
HI @manick02 -
Thanks for the pull request. Here are a few things to keep in mind:
- Please check off the boxes if you've completed the item.
- Please clarify which module you ran the test on. For this particular change, I think we would need to run tests on zookeeper-api, helix-core, and helix-rest modules.
- In general, every logical change must be accompanied by a corresponding test. Do you think you could add a test testing the change in logic?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1292:
URL: https://github.com/apache/helix/pull/1292#issuecomment-676669851
> > I am trying understand why this test is failing in CI but passing locally
>
> > java.lang.AssertionError: expected:<0> but was:<1>
>
> > at org.apache.helix.zookeeper.impl.client.TestRawZkClient.testZkClientMonitor(TestRawZkClient.java:288)
>
> >
>
> > [INFO]
>
> > [INFO] Results:
>
> > [INFO]
>
> > [ERROR] Failures:
>
> > [ERROR] TestRawZkClient.testZkClientMonitor:288 expected:<0> but was:<1>
>
> > [INFO]
>
> > [ERROR] Tests run: 44, Failures: 1, Errors: 0, Skipped: 2
>
>
>
> @jiajunwang @pkuwm @kaisun2000 is this test a failing item in CI? Has our CI been made stable yet?
@narendly From the ci result, it is still a failure. Being successful in local is because the ordering of tests running is different with CI. I believe @kaisun2000 is working on fixing the failed tests. Maybe @kaisun2000 could prioritize this test fix so zookeeper api module can pass tests?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1292:
URL: https://github.com/apache/helix/pull/1292#discussion_r488402272
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##########
@@ -116,7 +116,10 @@ private ZkConnectionManager getOrCreateZkConnectionManager(
// to avoid race condition.
private void cleanupConnectionManager(ZkConnectionManager zkConnectionManager) {
synchronized (_connectionManagerPool) {
- zkConnectionManager.close(true);
+ boolean canClose = zkConnectionManager.close(true);
Review comment:
nit, canClose => connectionClosed
Or maybe simpler,
if (zkConnectionManager.close(true)) {
_connectionManagerPool.remove(zkConnectionManager);
}
##########
File path: zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestSharedZkClient.java
##########
@@ -66,4 +65,16 @@ public void testCreateEphemeralFailure() {
// this is expected.
}
}
+
+ /*
+ Tests clean up of closed connection from pool
+ Use cases tested
+ -Not removal of connection when there is a active watchers
+ -Removal of connection when there are no active watchers
+ */
+ @Test
+ public void testCleanupOfClosedConnectionFromConnectionManager(){
Review comment:
One way that I can think of is modifying the test only method getActiveConnectionCount() so it can return either the total connection manager count or active manager count according to an input parameter.
My only concern is that I don't like the idea of making these test methods more complicated : ( However, it might be the easiest way.
Please try to think of any better idea. If we really don't have anything better, upgrading getActiveConnectionCount is still an OK option.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
narendly commented on pull request #1292:
URL: https://github.com/apache/helix/pull/1292#issuecomment-676636142
> I am trying understand why this test is failing in CI but passing locally
> java.lang.AssertionError: expected:<0> but was:<1>
> at org.apache.helix.zookeeper.impl.client.TestRawZkClient.testZkClientMonitor(TestRawZkClient.java:288)
>
> [INFO]
> [INFO] Results:
> [INFO]
> [ERROR] Failures:
> [ERROR] TestRawZkClient.testZkClientMonitor:288 expected:<0> but was:<1>
> [INFO]
> [ERROR] Tests run: 44, Failures: 1, Errors: 0, Skipped: 2
@jiajunwang @pkuwm @kaisun2000 is this test a failing item in CI? Has our CI been made stable yet?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1292:
URL: https://github.com/apache/helix/pull/1292#issuecomment-834666067
@manick02 Any update please? Or I will close this PR for now due to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] manick02 commented on a change in pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
manick02 commented on a change in pull request #1292:
URL: https://github.com/apache/helix/pull/1292#discussion_r483528917
##########
File path: zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestSharedZkClient.java
##########
@@ -66,4 +65,16 @@ public void testCreateEphemeralFailure() {
// this is expected.
}
}
+
+ /*
+ Tests clean up of closed connection from pool
+ Use cases tested
+ -Not removal of connection when there is a active watchers
+ -Removal of connection when there are no active watchers
+ */
+ @Test
+ public void testCleanupOfClosedConnectionFromConnectionManager(){
Review comment:
Actually, am bit stuck with how to test this and i was wondering if someone can guide me here. I posted it here - https://github.com/apache/helix/pull/1292#issuecomment-680828563
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] manick02 commented on a change in pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
manick02 commented on a change in pull request #1292:
URL: https://github.com/apache/helix/pull/1292#discussion_r483518438
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##########
@@ -117,6 +117,9 @@ private ZkConnectionManager getOrCreateZkConnectionManager(
private void cleanupConnectionManager(ZkConnectionManager zkConnectionManager) {
synchronized (_connectionManagerPool) {
zkConnectionManager.close(true);
+ if (!zkConnectionManager.hasSharedWatchers()) {
Review comment:
Thanks for the feedback, will incorporate this
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] manick02 commented on pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
manick02 commented on pull request #1292:
URL: https://github.com/apache/helix/pull/1292#issuecomment-676856162
> HI @manick02 -
>
> Thanks for the pull request. Here are a few things to keep in mind:
>
> * Please check off the boxes if you've completed the item.
> * Please clarify which module you ran the test on. For this particular change, I think we would need to run tests on zookeeper-api, helix-core, and helix-rest modules.
> * In general, every logical change must be accompanied by a corresponding test. Do you think you could add a test testing the change in logic?
Thanks for the comments. I am figuring out to how to test this, let me get back
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] manick02 commented on pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
manick02 commented on pull request #1292:
URL: https://github.com/apache/helix/pull/1292#issuecomment-680828563
Am bit stuck here i though i will reach out here and look for guidance. Initially, i thought i will add my test in TestSharedZkClient, but this test class extends RealmAwareZkClientTestBase (this test has its own test suite initialisation). This made me wonder what is the right place to test for this issue. Following is what am thinking of doing, start zookeeper create shared clients and close it to see if it is getting removed ( variants : one watcher, more than one watcher). Any Guidance will be much appreciated and will help me proceed, not sure what is the right channel to discuss also so i thought i will ask here
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang closed pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
jiajunwang closed pull request #1292:
URL: https://github.com/apache/helix/pull/1292
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] manick02 commented on a change in pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
manick02 commented on a change in pull request #1292:
URL: https://github.com/apache/helix/pull/1292#discussion_r473546955
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##########
@@ -117,6 +117,7 @@ private ZkConnectionManager getOrCreateZkConnectionManager(
private void cleanupConnectionManager(ZkConnectionManager zkConnectionManager) {
synchronized (_connectionManagerPool) {
zkConnectionManager.close(true);
Review comment:
Thank you, i will understand what condition needs to be taken care before removing from the pool and come back and propose
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1292:
URL: https://github.com/apache/helix/pull/1292#discussion_r483234049
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/ZkConnectionManager.java
##########
@@ -48,12 +47,12 @@
* While multiple Shared ZkClients can use single connection manager if possible.
*/
public class ZkConnectionManager extends ZkClient {
- private static Logger LOG = LoggerFactory.getLogger(ZkConnectionManager.class);
// Client type that is used in monitor, and metrics.
private final static String MONITOR_TYPE = "ZkConnectionManager";
- private final String _monitorKey;
+ private static Logger LOG = LoggerFactory.getLogger(ZkConnectionManager.class);
Review comment:
nit, please avoid unnecessary changes.
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##########
@@ -117,6 +117,9 @@ private ZkConnectionManager getOrCreateZkConnectionManager(
private void cleanupConnectionManager(ZkConnectionManager zkConnectionManager) {
synchronized (_connectionManagerPool) {
zkConnectionManager.close(true);
+ if (!zkConnectionManager.hasSharedWatchers()) {
Review comment:
This shall work, but will it be easier if we let close() return a boolean to indicate if the real close has been done? If true, then it is safe to remove it from the pool.
It is both easier and safer because once a zkConnectionManager is closed, there is no way to reuse it.
So even if later we modify the concurrency control, there is no risk at all.
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/ZkConnectionManager.java
##########
@@ -132,6 +131,10 @@ protected synchronized void close(boolean skipIfWatched) {
LOG.info("ZkConnection {} was closed.", _monitorKey);
}
+ protected boolean hasSharedWatchers() {
Review comment:
Please change the other logic that doing the same check refers to this method.
##########
File path: zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestSharedZkClient.java
##########
@@ -66,4 +65,16 @@ public void testCreateEphemeralFailure() {
// this is expected.
}
}
+
+ /*
+ Tests clean up of closed connection from pool
+ Use cases tested
+ -Not removal of connection when there is a active watchers
+ -Removal of connection when there are no active watchers
+ */
+ @Test
+ public void testCleanupOfClosedConnectionFromConnectionManager(){
Review comment:
Please finish the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] manick02 commented on pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
manick02 commented on pull request #1292:
URL: https://github.com/apache/helix/pull/1292#issuecomment-675876326
I am trying understand why this test is failing in CI but passing locally
java.lang.AssertionError: expected:<0> but was:<1>
at org.apache.helix.zookeeper.impl.client.TestRawZkClient.testZkClientMonitor(TestRawZkClient.java:288)
[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestRawZkClient.testZkClientMonitor:288 expected:<0> but was:<1>
[INFO]
[ERROR] Tests run: 44, Failures: 1, Errors: 0, Skipped: 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] manick02 edited a comment on pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
manick02 edited a comment on pull request #1292:
URL: https://github.com/apache/helix/pull/1292#issuecomment-676856162
> HI @manick02 -
>
> Thanks for the pull request. Here are a few things to keep in mind:
>
> * Please check off the boxes if you've completed the item.
> * Please clarify which module you ran the test on. For this particular change, I think we would need to run tests on zookeeper-api, helix-core, and helix-rest modules.
> * In general, every logical change must be accompanied by a corresponding test. Do you think you could add a test testing the change in logic?
Thanks for the comments. I am figuring out to how to test this, let me get back
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1292:
URL: https://github.com/apache/helix/pull/1292#discussion_r473364596
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##########
@@ -117,6 +117,7 @@ private ZkConnectionManager getOrCreateZkConnectionManager(
private void cleanupConnectionManager(ZkConnectionManager zkConnectionManager) {
synchronized (_connectionManagerPool) {
zkConnectionManager.close(true);
Review comment:
When skipIfWatched == true, the close() call may not actually disconnect the zkConnectionManager given shared watchers exist. So we shall not remove the zkConnectionManager from the pool without a proper check.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1292:
URL: https://github.com/apache/helix/pull/1292#issuecomment-676782811
Will file a task to fix this one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1292:
URL: https://github.com/apache/helix/pull/1292#issuecomment-838852409
Close due to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on pull request #1292: Remove closed ZKConnectionManager from connection manager pool
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1292:
URL: https://github.com/apache/helix/pull/1292#issuecomment-676782587
@huizhi, @narendly I guess I may find a theory as why this test fails in github, but not locally.
See the following code, there is a race condition here. `connect()` would cause a process() callback in zookeeper object eventThread.. If the _monitor is not constructed, the state changed value would be 0, otherwise, it would be 1.
github is slow, the value is 1.
```
protected ZkClient(IZkConnection zkConnection, int connectionTimeout, long operationRetryTimeout,
PathBasedZkSerializer zkSerializer, String monitorType, String monitorKey,
String monitorInstanceName, boolean monitorRootPathOnly) {
if (zkConnection == null) {
throw new NullPointerException("Zookeeper connection is null!");
}
_uid = UID.getAndIncrement();
_connection = zkConnection;
_pathBasedZkSerializer = zkSerializer;
_operationRetryTimeoutInMillis = operationRetryTimeout;
_isNewSessionEventFired = false;
_asyncCallRetryThread = new ZkAsyncRetryThread(zkConnection.getServers());
_asyncCallRetryThread.start();
LOG.info("ZkClient created with _uid {}, _asyncCallRetryThread id {}, stack {}", _uid, _asyncCallRetryThread.getId(),
Arrays.asList(Thread.currentThread().getStackTrace()));
connect(connectionTimeout, this);
// initiate monitor
try {
if (monitorKey != null && !monitorKey.isEmpty() && monitorType != null && !monitorType
.isEmpty()) {
_monitor =
new ZkClientMonitor(monitorType, monitorKey, monitorInstanceName, monitorRootPathOnly,
_eventThread);
_monitor.register();
} else {
LOG.info("ZkClient monitor key or type is not provided. Skip monitoring.");
}
} catch (JMException e) {
LOG.error("Error in creating ZkClientMonitor", e);
}
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org