You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "XComp (via GitHub)" <gi...@apache.org> on 2023/11/22 15:35:16 UTC
[PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
XComp opened a new pull request, #23773:
URL: https://github.com/apache/flink/pull/23773
## What is the purpose of the change
This is a follow-up of [PR #23415](https://github.com/apache/flink/pull/23415) which tries to come up with a more general solution for working around the thread leakage on the ZooKeeper server side. The motivation was that the approach which was introduced in #23415 changed some visibility constraints that are not really desired (making the `ZooKeeperLeaderRetrievalDriver` aware of the `ResourceManager`).
This change is meant as a draft and should rather be used as a base for a discussion whether FLINK-33053 is actually an issue that should be addressed in Flink itself. As far as I understand, the thread leakage is only happening in test code when the ZooKeeper test server implementation is set up.
## Brief change log
* Introduced a reference counter in the `ZooKeeperLeaderRetrievalFactory` that counts the number of instances that are created by this factory instance. This reference counter is passed into all driver instances and is used to decide whether the `removeAll` call should be performed during `close()`.
## Verifying this change
I tried to come up with a test to verify the behavior. But it's hard to test because the thread leakage is actually happening in the ZooKeeper server itself.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: yes
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #23773: [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side.
URL: https://github.com/apache/flink/pull/23773
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23773:
URL: https://github.com/apache/flink/pull/23773#issuecomment-2777668024
This PR is being marked as stale since it has not had any activity in the last 90 days.
If you would like to keep this PR alive, please leave a comment asking for a review.
If the PR has merge conflicts, update it with the latest from the base branch.
If you are having difficulty finding a reviewer, please reach out to the
community, contact details can be found here: https://flink.apache.org/what-is-flink/community/
If this PR is no longer valid or desired, please feel free to close it.
If no activity occurs in the next 30 days, it will be automatically 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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "KarmaGYZ (via GitHub)" <gi...@apache.org>.
KarmaGYZ commented on PR #23773:
URL: https://github.com/apache/flink/pull/23773#issuecomment-1823701763
Thanks for the fix, @XComp . Great job!
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "XComp (via GitHub)" <gi...@apache.org>.
XComp commented on code in PR #23773:
URL: https://github.com/apache/flink/pull/23773#discussion_r1402250278
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriver.java:
##########
@@ -69,8 +68,40 @@ public class ZooKeeperLeaderRetrievalDriver implements LeaderRetrievalDriver {
private final FatalErrorHandler fatalErrorHandler;
+ /**
+ * Each {@code ZooKeeperLeaderRetrievalDriver} has its own watcher initialized. There is a bug
Review Comment:
> This change is meant as a draft and should rather be used as a base for a discussion whether [FLINK-33053](https://issues.apache.org/jira/browse/FLINK-33053) is actually an issue that should be addressed in Flink itself. As far as I understand, the thread leakage is only happening in test code when the ZooKeeper test server implementation is set up.
@KarmaGYZ Is it correct that the thread leakage happens in ZooKeeper's TestServer implementation? :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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "XComp (via GitHub)" <gi...@apache.org>.
XComp commented on code in PR #23773:
URL: https://github.com/apache/flink/pull/23773#discussion_r1403018967
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriver.java:
##########
@@ -126,17 +161,13 @@ public void close() throws Exception {
cache.close();
- try {
- if (client.getZookeeperClient().isConnected()
- && !connectionInformationPath.contains(RESOURCE_MANAGER_NODE)) {
- client.watchers()
- .removeAll()
- .ofType(Watcher.WatcherType.Any)
- .forPath(connectionInformationPath);
- }
- } catch (KeeperException.NoWatcherException e) {
- // Ignore the no watcher exception as it's just a safetynet to fix watcher leak issue.
- // For more details, please refer to FLINK-33053.
+ if (client.getZookeeperClient().isConnected()
+ && watcherReferenceCounter.getAndIncrement() == 1) {
Review Comment:
yikes, I introduced that one when I did another code change. Good catch :+1: That proofs once more that a proper test should be added to cover this code path. :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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "KarmaGYZ (via GitHub)" <gi...@apache.org>.
KarmaGYZ commented on code in PR #23773:
URL: https://github.com/apache/flink/pull/23773#discussion_r1403026090
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriver.java:
##########
@@ -69,8 +68,40 @@ public class ZooKeeperLeaderRetrievalDriver implements LeaderRetrievalDriver {
private final FatalErrorHandler fatalErrorHandler;
+ /**
+ * Each {@code ZooKeeperLeaderRetrievalDriver} has its own watcher initialized. There is a bug
Review Comment:
No, I did the stress tests with a real ZK cluster. So, I'm afraid we might need an e2e test for it.
The leackage is happened in the ZK side. Your fix loods reasonable to me, at least for the current HA mechanism.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "XComp (via GitHub)" <gi...@apache.org>.
XComp commented on code in PR #23773:
URL: https://github.com/apache/flink/pull/23773#discussion_r1403041122
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriver.java:
##########
@@ -69,8 +68,40 @@ public class ZooKeeperLeaderRetrievalDriver implements LeaderRetrievalDriver {
private final FatalErrorHandler fatalErrorHandler;
+ /**
+ * Each {@code ZooKeeperLeaderRetrievalDriver} has its own watcher initialized. There is a bug
Review Comment:
hm, ok ... thanks for clarification. But that brings me to the point where I would be in favor of not fixing it on Flink's side at all. That needs to be addressed in ZooKeeper (as @tisonkun pointed out with his reference to ZOOKEEPER-4625). I'm curious to hear what your take on this is?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "XComp (via GitHub)" <gi...@apache.org>.
XComp commented on code in PR #23773:
URL: https://github.com/apache/flink/pull/23773#discussion_r1403041122
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriver.java:
##########
@@ -69,8 +68,40 @@ public class ZooKeeperLeaderRetrievalDriver implements LeaderRetrievalDriver {
private final FatalErrorHandler fatalErrorHandler;
+ /**
+ * Each {@code ZooKeeperLeaderRetrievalDriver} has its own watcher initialized. There is a bug
Review Comment:
hm, ok ... thanks for clarification. But that brings me to the point where I would be in favor of not fixing it on Flink's side at all. That needs to be addressed in ZooKeeper (as @tisonkun pointed out with his reference to [ZOOKEEPER-4625](https://issues.apache.org/jira/browse/ZOOKEEPER-4625)). I'm curious to hear what your take on this is?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23773:
URL: https://github.com/apache/flink/pull/23773#issuecomment-2850023760
This PR has been closed since it has not had any activity in 120 days.
If you feel like this was a mistake, or you would like to continue working on it,
please feel free to re-open the PR and ask for 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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "KarmaGYZ (via GitHub)" <gi...@apache.org>.
KarmaGYZ commented on code in PR #23773:
URL: https://github.com/apache/flink/pull/23773#discussion_r1403065234
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriver.java:
##########
@@ -69,8 +68,40 @@ public class ZooKeeperLeaderRetrievalDriver implements LeaderRetrievalDriver {
private final FatalErrorHandler fatalErrorHandler;
+ /**
+ * Each {@code ZooKeeperLeaderRetrievalDriver} has its own watcher initialized. There is a bug
Review Comment:
I think the core issue is a specific component should take the responsibility to manage the lifecycle of watches. This component can be ZooKeeper, Curator, or Flink. ZK can provide a "closeWatch" interface to reconcile watch references on the server-side. Similarly, Curator and Flink can proactively handle this task and clean up by using "closeAll," just like what you did in this PR.
I personally believe that let ZK to handle this task is more reasonable. However, considering the release cycles of ZK and the need to maintain compatibility with multiple versions of ZK in Flink, we may need to proactively manage the lifecycle of watches within Flink in the foreseeable future.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "XComp (via GitHub)" <gi...@apache.org>.
XComp commented on code in PR #23773:
URL: https://github.com/apache/flink/pull/23773#discussion_r1403041122
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriver.java:
##########
@@ -69,8 +68,40 @@ public class ZooKeeperLeaderRetrievalDriver implements LeaderRetrievalDriver {
private final FatalErrorHandler fatalErrorHandler;
+ /**
+ * Each {@code ZooKeeperLeaderRetrievalDriver} has its own watcher initialized. There is a bug
Review Comment:
hm, ok ... thanks for clarification. But that brings me to the point where I would be in favor of not fixing it on Flink's side at all. That needs to be addressed in ZooKeeper (as @tisonkun pointed out with his reference to ZOOKEEPER-4625). What's your take on 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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "KarmaGYZ (via GitHub)" <gi...@apache.org>.
KarmaGYZ commented on code in PR #23773:
URL: https://github.com/apache/flink/pull/23773#discussion_r1402826269
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriverFactory.java:
##########
@@ -46,11 +51,13 @@ public ZooKeeperLeaderRetrievalDriverFactory(
public ZooKeeperLeaderRetrievalDriver createLeaderRetrievalDriver(
LeaderRetrievalEventHandler leaderEventHandler, FatalErrorHandler fatalErrorHandler)
throws Exception {
+ driverReferenceCounter.incrementAndGet();
Review Comment:
I think each retrievalPath should have a seperate counter, do I understand it correctly?
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriver.java:
##########
@@ -69,8 +68,40 @@ public class ZooKeeperLeaderRetrievalDriver implements LeaderRetrievalDriver {
private final FatalErrorHandler fatalErrorHandler;
+ /**
+ * Each {@code ZooKeeperLeaderRetrievalDriver} has its own watcher initialized. There is a bug
Review Comment:
Sorry, I'm not familiar with it. At least I can't find a public api to retrieve the watch info. Maybe you can verify with [this guide](https://kb.altinity.com/altinity-kb-setup-and-maintenance/altinity-kb-zookeeper/altinity-kb-how-to-check-the-list-of-watches/) to see whether we can get the watches of TestServer and wether there are leakage.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriver.java:
##########
@@ -126,17 +161,13 @@ public void close() throws Exception {
cache.close();
- try {
- if (client.getZookeeperClient().isConnected()
- && !connectionInformationPath.contains(RESOURCE_MANAGER_NODE)) {
- client.watchers()
- .removeAll()
- .ofType(Watcher.WatcherType.Any)
- .forPath(connectionInformationPath);
- }
- } catch (KeeperException.NoWatcherException e) {
- // Ignore the no watcher exception as it's just a safetynet to fix watcher leak issue.
- // For more details, please refer to FLINK-33053.
+ if (client.getZookeeperClient().isConnected()
+ && watcherReferenceCounter.getAndIncrement() == 1) {
Review Comment:
getAndDecrement?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #23773:
URL: https://github.com/apache/flink/pull/23773#issuecomment-1823007310
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c73832ddb8476e8d206667f060aabde0aacedfc9",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "c73832ddb8476e8d206667f060aabde0aacedfc9",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* c73832ddb8476e8d206667f060aabde0aacedfc9 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "XComp (via GitHub)" <gi...@apache.org>.
XComp commented on code in PR #23773:
URL: https://github.com/apache/flink/pull/23773#discussion_r1403015737
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriver.java:
##########
@@ -69,8 +68,40 @@ public class ZooKeeperLeaderRetrievalDriver implements LeaderRetrievalDriver {
private final FatalErrorHandler fatalErrorHandler;
+ /**
+ * Each {@code ZooKeeperLeaderRetrievalDriver} has its own watcher initialized. There is a bug
Review Comment:
I'm just asking because you mentioned in the description of FLINK-33053 that you observed the issue in stress tests. Did you use the MiniCluster and ZooKeeper in a single JVM or did you have your ZooKeeper deployed separately for the stress tests?
I'd like to understand whether it's on the ZooKeeper side (that's how it sounds to me right now) or on the Flink/Curator side. If I misunderstood the discussion in FLINK-33053 and the related PR #23415 and it's not a ZooKeeper server issue, fixing it on the Flink side is reasonable.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33053][runtime] Refactoring of workaround to fix thread leakage on the ZooKeeper server side. [flink]
Posted by "XComp (via GitHub)" <gi...@apache.org>.
XComp commented on code in PR #23773:
URL: https://github.com/apache/flink/pull/23773#discussion_r1403016291
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriverFactory.java:
##########
@@ -46,11 +51,13 @@ public ZooKeeperLeaderRetrievalDriverFactory(
public ZooKeeperLeaderRetrievalDriver createLeaderRetrievalDriver(
LeaderRetrievalEventHandler leaderEventHandler, FatalErrorHandler fatalErrorHandler)
throws Exception {
+ driverReferenceCounter.incrementAndGet();
Review Comment:
yes, because there's one `ZooKeeperLeaderRetrievalFactory` per path.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org