[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-885343922 Thanks @wangyang0918 , @dmvk, and @tillrohrmann . Thanks for your help to make it happen. Glad to participate in improving Flink. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-884598878 Hi @dmvk @tillrohrmann , i resolved the conflict again, haha. Would you give it a final pass? :) -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-883972520 Thanks @dmvk , i think i've fixed all of the comments. PTAL -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-883958117 @dmvk the quick applied suggestion misses to remove the initialization in the constructor. So i fixed it and rebase it to 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-883824268 @dmvk @wangyang0918 Please take another 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-881993758 @dmvk it seems that the global uncaught event handler not work in test? The last tests fails because a NPE is thrown in `EventHandler`, but the test fails since the single thread executor service terminated and no executor for the new events instead of global exiting. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-881883388 @flinkbot run azure -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-881566693 @dmvk I have push the new changes: 1. watch with executor service, share a cached pool in the ha service; 2. run add/update/delete event directly > One thing worth mentioning is that, in `EventHandler.addWatch` use local cached resource instead of get from indexer because the getting result may have not be dispatched to the current handler. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-881349133 It then can not be guaranteed each events executed one by one. But it seems ok for the current config map watching usage, In general, the executor is passed from the caller, the caller can determined how to execute the event. If it need execute events one by one, then it should pass a single thread to the watcher. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-881339246 @dmvk I' wondering whether we can only implement the interface `Watch watch(String name, WatchCallbackHandler callbackHandler, ExecutorService callbackExecutor);` in KubernetesSharedInformer, and use it if needed in the future. Or we should give an executor to for the current each watching, which will lead to a thread for each watching ? -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-881332432 @dmvk Oh sorry, i misunderstood that we can improving the asynchronous callback execution in another change, say changing the watch method by adding an executor argument. It's ok to address it here, so let's do 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-881288825 @dmvk @wangyang0918 I've synced with the master branch, would you have another 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-878990690 @dmvk @wangyang0918 i missed this discussion before pushing new change. Fortunately, looks like the implementation doesn't need to change, :) A little different from the draft by David: the executor service passed to informer is not used to handle event. So we still need to submit event manually. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-878904369 > I'm pretty sure that single thread, that you already use for handling events in informer prior putting them into queue, should be enough for handling watchers as well. @dmvk Thanks David for pointing out this, it will be pretty good for me if a single thread is enough for handling all events. Would you merge your single thread edition? Or let me push another change later. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-864905819 @tillrohrmann Do you have some more time for this 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-864905819 @tillrohrmann Do you have some more time for this 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-849258434 Hi @wangyang0918 , i've added the comment why we use Long.MAX_VALUE. Furthermore, i add a listener to deal with the exception thrown by the informer. -- 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: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-847689225 > However, I still find a suspicious log which shows up every 5 seconds. It might be related with [fabric8io/kubernetes-client#2651](https://github.com/fabric8io/kubernetes-client/issues/2651). This will greatly increase the pressure of ETCD and affect the whole K8s cluster stability. Could you please have a look? > > ``` > 2021-05-25 07:04:45,580 DEBUG [pool-5-thread-1] io.fabric8.kubernetes.client.informers.cache.Reflector [] - Listing items (4) for resource class io.fabric8.kubernetes.api.model.ConfigMap v3656102 > ``` I missed the DEFAULT_PERIOD before, how about passing the `Long.MAX_VALUE` to walk around 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-847675307 Thanks @wangyang0918 , i will check the periodical resync. And add the doc update later. -- 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: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-845823331 Thanks @wangyang0918 , i have updated the commit. -- 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: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-845629668 If the issue is the potential risk of introducing a complex dependency, i think it may be a way by adding a feature gate and refactoring the original adhoc watching like the current way, and the feature gate is used to control how to create the Watcher. And the original watcher will create a watch for each of watching, and the new one is based on informer. -- 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: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-845625212 @tillrohrmann Would you help to make this PR continue? -- 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: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-842765986 Hi @wangyang0918 , does this change still make sense? -- 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: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-834022020 @wangyang0918 @tillrohrmann Please have another 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-818403355 Hi @wangyang0918 @tillrohrmann , So what do you think in summary? * using shared indexed informer or not? * provide a new interface to replace the old 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: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-816485868 > I am also not suggesting to implement our own shared watch. It just feels like we are doing some duplicated things that shared informer already supports. One thing we need to implement is the event dispatcher whether we using the informer or not. The `ResourceEventHandler` is not proper for our use case. If we want to provide a watcher for the special case( only care about MODIFIED, and it will always occur as described before ), a dispatcher with the normal watch is ok. it's simple but not another simplified informer. If we use the informer, we can provide a close-to-real watching. It's ok for me to provide a new individual interface, no matter how we achieve 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-816352833 @wangyang0918 Thank you for your timely feedback so that it will not be too far if we are in the wrong direction. > * Except for saving the web socket connections, I am not sure what else we could gain by using shared informer over separate watches. I am just uncertain to use such a powerful but complex feature. > Yeah, it's powerful but complex. So why i use it and implement like this, to be short, i want to make it act like the normal watching Let me explain it in detail: Say we are running a shared watching now, a new watching for CM-a is requested now. An ADDED event should occur (if not existed before), and followed by some MODIFIED events if it is updated, (then deleted). We can not get the ADDED event from the shared watching if it is already existed, . So can we add an ADDED event by fetching from K8s API separately? It seems not correct, we cannot guarantee the timing of these two approach: duplicated ADDED events or ADDED after MODIFIED. So we should get the object from a place which is sequential with the events. And looks like the informer is the proper way, brings some added value(like reconnecting) at the same time. **BUT** > * Because `FlinkKubeClient#watchConfigMaps` is only used for Kubernetes HA services. If we just use it for Kubernetes HA services and watching those updated periodically Leader ConfigMap, and we don't care whether the ADDED event occur. We can just using a simple normal watching like before, and dispatch events just as they actually came out. > Compared with keeping the current `FlinkKubeClient#watchConfigMaps` interface, I prefer to remove it and add a new interface for creating shared informer. And if we don't care about the original interface, create another for shared watching is ok. > Actually, we completely change the behavior about how to start the watch, do the callback, even recreate the watch when failed. The implementation here is expected to keep the behavior like before. But if we don't need to keep it because of the special use case (only for those Leader ConfigMaps), it's ok to me. > > ``` > KubernetesSharedInformer createKubernetesConfigMapSharedInformer(Map labels); > ``` > > * Just like `KubernetesLeaderElector`, maybe `Fabric8FlinkKubeConfigMapWatcher` could be renamed to `KubernetesSharedInformer` and put in `org.apache.flink.kubernetes.kubeclient.resources`. > * We could create/close HA related ConfigMap sharedInformer in `KubernetesHaServices` and pass it to `KubernetesLeaderElectionDriver` and `KubernetesLeaderRetrievalDriver`. Then the driver will register the event handler. And we can add new interface and it is the same simply watching like watching single ConfigMap: ``` KubernetesWatch FlinkKubeClient#watchConfigMaps(Map labels, Handler handler); ``` And manage it in `KubernetesHaServices`. > * I am wondering whether we could share a `Processor` for same ConfigMap with different event handlers in `Fabric8FlinkKubeConfigMapWatcher` if the performance allows. Each ConfigMap just needs a dedicated `Processor` to process the events sequentially. Then we can share a `Processor` for same ConfigMap. And @tillrohrmann, what's your opinion? -- 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: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-815422875 @flinkbot run azure -- 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: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-814969966 Thanks, @tillrohrmann , @wangyang0918 > There seems to be a compilation error Yeah , i didn't realize the local CheckStyle plugin does not work. I have added another fix commit. -- 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: us...@infra.apache.org
[GitHub] [flink] yittg commented on pull request #15501: [FLINK-22054][k8s] Using a shared watcher for ConfigMap watching
yittg commented on pull request #15501: URL: https://github.com/apache/flink/pull/15501#issuecomment-814711256 @flinkbot attention @tillrohrmann -- 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: us...@infra.apache.org
