Hello Matthias, Thanks a lot for the feedback.
- Regarding names: sure it totally makes sense to follow the kebab case and Flip has reflected the change. Regarding the convention, Flink has this widely used configuration storageDir, which doesn't follow the kebab rule and creates some confusion. IMHO it would be valuable to add a clear guide. - I am considering calling the next method from the Curator framework: authorization(List<AuthInfo>) [2]. I have added necessary details regarding Map<String, String> -> List(AuthInfo) conversion, taking into account that AuthInfo has a constructor with String, byte[] parameters. - Good point. Please let me know if I am missing something, but it seems that we already can influence ACLProvider for Curator in Flink with high-availability.zookeeper.client.acl [2] . The way it is done currently is translation of the predefined constant to some predefined ACL Provider [3]. I do not see if we can add something to the current FLIP. I suppose that eventual extension of the supported ACLProvider would be straightforward and could be done outside of the current Flip as soon as concrete use-case requirements arise. Kind Regards Oleksandr [1] https://curator.apache.org/apidocs/org/apache/curator/framework/CuratorFrameworkFactory.Builder.html#authorization(java.util.List)https://curator.apache.org/apidocs/org/apache/curator/framework/CuratorFrameworkFactory.Builder.html#authorization(java.util.List) <https://curator.apache.org/apidocs/org/apache/curator/framework/CuratorFrameworkFactory.Builder.html#authorization(java.util.List)> [2] https://nightlies.apache.org/flink/flink-docs-release-1.18/docs/deployment/config/#high-availability-zookeeper-client-acl [3] https://github.com/apache/flink/blob/master/flink-runtime/src/main/java/org/apache/flink/runtime/util/ZooKeeperUtils.java#L208 On Mon, Jan 22, 2024 at 1:10 PM Matthias Pohl <matthias.p...@aiven.io.invalid> wrote: > Hi Alex, > thanks for bringing this up and sorry for jumping into the discussion that > late. Exposing additional curator configuration parameters through Flink's > config sounds reasonable. Overall, I am fine with the selection of > parameters. My judgement is entirely based on the curator > description, though. If someone has more experience with > configuring/deploying ZooKeeper; it would be great to get more insights. > > Here are a few remarks: > - You propose to use camel-case parameter names. I tried to find some > documentation where we decide on the actual formatting for configuration > parameters. Unfortunately, I didn't find any. The Flink configuration > appears to follow kebap-casing, though. We might want to use this format in > this FLIP as well. > > A bit out-of-scope for this FLIP: Is the configuration parameter format > documented somewhere? Should we add such a rule to the coding conventions > [1]. What do others think? > > - You mention the authentication parameter which expects byte[]. You > suggest using getBytes() to transform the value from the Flink > configuration "Map<String, String>" to what Curator expects. Can you > elaborate in the FLIP a bit more how the overall configuration would look > like in the end? I assume you refer to ConfigOptions#mapType when talking > about Map<String, String>? Do you want to allow multiple AuthInfo records > to be configurable? > > - In [2], I found an example where they also suggest utilizing the > AclProvider for allowing write access. This wouldn't be necessary in our > case because the credentials are managed by an outside process, I guess? Or > do we need to look into supporting AclProviders as well? > > Best, > Matthias > > [1] > > https://flink.apache.org/how-to-contribute/code-style-and-quality-formatting/#naming > [2] https://stackoverflow.com/a/55171761 > > > On Mon, Jan 8, 2024 at 9:46 AM Alex Nitavsky <alexnitav...@gmail.com> > wrote: > > > Thanks Zhanghao Chen for the feedback. > > > > In general right now there is no way to authorise Flink via the Curator > > framework, which is probably an important feature which is missing. Since > > public configuration change requires a Flip, current Flip is proposing to > > add several more potentially important Curator configurations. > > > > Kind regards > > Oleksandr > > > > On Thu, Jan 4, 2024 at 4:57 AM Zhanghao Chen <zhanghao.c...@outlook.com> > > wrote: > > > > > Thanks for driving this. I'm not familiar with the listed advanced > > Curator > > > configs, but the previous added switch for disabling ensemble tracking > > [1] > > > saved us when deploying Flink in a cloud env where ZK can only be > > > accessible via URLs. That being said, +1 for the overall idea, these > > > configs may help users in certain scenarios sooner or later. > > > > > > [1] https://issues.apache.org/jira/browse/FLINK-31780 > > > > > > Best, > > > Zhanghao Chen > > > ________________________________ > > > From: Alex Nitavsky <alexnitav...@gmail.com> > > > Sent: Thursday, December 14, 2023 21:20 > > > To: dev@flink.apache.org <dev@flink.apache.org> > > > Subject: [DISCUSS] FLIP-402: Extend ZooKeeper Curator configurations > > > > > > Hi all, > > > > > > I would like to start a discussion thread for: *FLIP-402: Extend > > ZooKeeper > > > Curator configurations *[1] > > > > > > * Problem statement * > > > Currently Flink misses several Apache Curator configurations, which > could > > > be useful for Flink deployment with ZooKeeper as HA provider. > > > > > > * Proposed solution * > > > We have inspected all possible options for Apache Curator and proposed > > > those which could be valuable for Flink users: > > > > > > - high-availability.zookeeper.client.authorization [2] > > > - high-availability.zookeeper.client.maxCloseWaitMs [3] > > > - high-availability.zookeeper.client.simulatedSessionExpirationPercent > > [4] > > > > > > The proposed way is to reflect those properties into Flink > configuration > > > options for Apache ZooKeeper. > > > > > > Looking forward to your feedback and suggestions. > > > > > > Kind regards > > > Oleksandr > > > > > > [1] > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-402%3A+Extend+ZooKeeper+Curator+configurations > > > [2] > > > > > > > > > https://curator.apache.org/apidocs/org/apache/curator/framework/CuratorFrameworkFactory.Builder.html#authorization(java.lang.String,byte%5B%5D) > > > [3] > > > > > > > > > https://curator.apache.org/apidocs/org/apache/curator/framework/CuratorFrameworkFactory.Builder.html#maxCloseWaitMs(int) > > > [4] > > > > > > > > > https://curator.apache.org/apidocs/org/apache/curator/framework/CuratorFrameworkFactory.Builder.html#simulatedSessionExpirationPercent(int) > > > > > >
