You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "1996fanrui (via GitHub)" <gi...@apache.org> on 2024/01/31 09:41:08 UTC

[PR] [FLINK-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

1996fanrui opened a new pull request, #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765

   Still under development.


-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1490275258


##########
flink-autoscaler-standalone/src/main/java/org/apache/flink/autoscaler/standalone/config/AutoscalerStandaloneOptions.java:
##########
@@ -109,4 +110,51 @@ private static ConfigOptions.OptionBuilder autoscalerStandaloneConfig(String key
                                                     + "export the password using this environment variable.",
                                             code(STATE_STORE_TYPE.key()), code(JDBC.toString()))
                                     .build());
+
+    public static final ConfigOption<EventHandlerType> EVENT_HANDLER_TYPE =
+            autoscalerStandaloneConfig("event-handler.type")
+                    .enumType(EventHandlerType.class)
+                    .defaultValue(EventHandlerType.LOGGING)
+                    .withDescription("The autoscaler event handler type.");

Review Comment:
   We can configure them separately.
   
   ```
   --autoscaler.standalone.state-store.type jdbc \
   --autoscaler.standalone.event-handler.type jdbc \
   ```
   
   After users choose JDBC for them, we can reuse all jdbc related options, such as:
   
   ```
   autoscaler.standalone.jdbc.username
   autoscaler.standalone.jdbc.url
   ```
   
   Here is an whole example(This example is from the doc):
   
   ```
   JDBC_DRIVER_JAR=./mysql-connector-java-8.0.30.jar
   # export the password of jdbc state store & jdbc event handler
   export JDBC_PWD=123456
   
   java -cp flink-autoscaler-standalone-{{< version >}}.jar:${JDBC_DRIVER_JAR} \
   org.apache.flink.autoscaler.standalone.StandaloneAutoscalerEntrypoint \
   --autoscaler.standalone.fetcher.flink-cluster.host localhost \
   --autoscaler.standalone.fetcher.flink-cluster.port 8081 \
   --autoscaler.standalone.state-store.type jdbc \
   --autoscaler.standalone.event-handler.type jdbc \
   --autoscaler.standalone.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
   --autoscaler.standalone.jdbc.username root
   ```



-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#issuecomment-1931498672

   Hi @gyfora @mxm , the development and test JdbcAutoscalerEventHandler are totally done,  it can be included in `1.8.0`.
   
   I finished the unit test, itcase, and I tested it on my local manually. 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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [FLINK-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "mxm (via GitHub)" <gi...@apache.org>.
mxm commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1489593700


##########
flink-autoscaler-standalone/src/main/java/org/apache/flink/autoscaler/standalone/config/AutoscalerStandaloneOptions.java:
##########
@@ -109,4 +110,51 @@ private static ConfigOptions.OptionBuilder autoscalerStandaloneConfig(String key
                                                     + "export the password using this environment variable.",
                                             code(STATE_STORE_TYPE.key()), code(JDBC.toString()))
                                     .build());
+
+    public static final ConfigOption<EventHandlerType> EVENT_HANDLER_TYPE =
+            autoscalerStandaloneConfig("event-handler.type")
+                    .enumType(EventHandlerType.class)
+                    .defaultValue(EventHandlerType.LOGGING)
+                    .withDescription("The autoscaler event handler type.");

Review Comment:
   What if users want to configure both logging and storing via JDBC?



-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1490275258


##########
flink-autoscaler-standalone/src/main/java/org/apache/flink/autoscaler/standalone/config/AutoscalerStandaloneOptions.java:
##########
@@ -109,4 +110,51 @@ private static ConfigOptions.OptionBuilder autoscalerStandaloneConfig(String key
                                                     + "export the password using this environment variable.",
                                             code(STATE_STORE_TYPE.key()), code(JDBC.toString()))
                                     .build());
+
+    public static final ConfigOption<EventHandlerType> EVENT_HANDLER_TYPE =
+            autoscalerStandaloneConfig("event-handler.type")
+                    .enumType(EventHandlerType.class)
+                    .defaultValue(EventHandlerType.LOGGING)
+                    .withDescription("The autoscaler event handler type.");

Review Comment:
   We can configure them separately.
   
   ```
   --autoscaler.standalone.state-store.type jdbc \
   --autoscaler.standalone.event-handler.type jdbc \
   ```
   
   After users choose JDBC for them, we can reuse all jdbc related options, such as:
   
   ```
   autoscaler.standalone.jdbc.username
   autoscaler.standalone.jdbc.url
   ```
   
   Here is an example:
   
   ```
   ```
   JDBC_DRIVER_JAR=./mysql-connector-java-8.0.30.jar
   # export the password of jdbc state store & jdbc event handler
   export JDBC_PWD=123456
   
   java -cp flink-autoscaler-standalone-{{< version >}}.jar:${JDBC_DRIVER_JAR} \
   org.apache.flink.autoscaler.standalone.StandaloneAutoscalerEntrypoint \
   --autoscaler.standalone.fetcher.flink-cluster.host localhost \
   --autoscaler.standalone.fetcher.flink-cluster.port 8081 \
   --autoscaler.standalone.state-store.type jdbc \
   --autoscaler.standalone.event-handler.type jdbc \
   --autoscaler.standalone.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
   --autoscaler.standalone.jdbc.username root
   ```
   ```
   



-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#issuecomment-1929131446

   @gyfora  @mxm , sorry to bother you again. During I supporting PostgreSQL and Derby, I found some defects for jdbc event handler and jdbc state store about the table schema.
   
   In order to ensure the stability of the table structure, I'd like to polish the state store design first. And finish the event handler in the next version. Thank you.


-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#issuecomment-1929257889

   > @1996fanrui How important is it for you to get this into 1.8.0? I think the 1.8.0 release could wait until then.
   
   If I didn't take vacation, I might ask for a two-day delay in release. (With two days of delay plus the weekend, I believe I have enough time to complete this PR). But I will be on vacation for 10 days, I am worried about affecting the release progress. Sorry for that.


-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1489463353


##########
docs/content/docs/custom-resource/autoscaler.md:
##########
@@ -286,16 +286,20 @@ please download JDBC driver and initialize database and table first.
 
 ```
 JDBC_DRIVER_JAR=./mysql-connector-java-8.0.30.jar
-# export the password of jdbc state store
+# export the password of jdbc state store & jdbc event handler
 export STATE_STORE_JDBC_PWD=123456
+export EVENT_HANDLER_JDBC_PWD=123456
 
 java -cp flink-autoscaler-standalone-{{< version >}}.jar:${JDBC_DRIVER_JAR} \
 org.apache.flink.autoscaler.standalone.StandaloneAutoscalerEntrypoint \
 --autoscaler.standalone.fetcher.flink-cluster.host localhost \
 --autoscaler.standalone.fetcher.flink-cluster.port 8081 \
 --autoscaler.standalone.state-store.type jdbc \
 --autoscaler.standalone.state-store.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
---autoscaler.standalone.state-store.jdbc.username root
+--autoscaler.standalone.state-store.jdbc.username root \
+--autoscaler.standalone.event-handler.type jdbc \
+--autoscaler.standalone.event-handler.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
+--autoscaler.standalone.event-handler.jdbc.username root

Review Comment:
   @1996fanrui I agree that we should probably merge the configs as they will be used together. Instead of calling it `jdbc-plugin` we could just call the properties:
   ```
   autoscaler.standalone.jdbc.username
   autoscaler.standalone.jdbc.url
   ...
   
   ```
   



-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1480045403


##########
docs/content/docs/custom-resource/autoscaler.md:
##########
@@ -286,16 +286,20 @@ please download JDBC driver and initialize database and table first.
 
 ```
 JDBC_DRIVER_JAR=./mysql-connector-java-8.0.30.jar
-# export the password of jdbc state store
+# export the password of jdbc state store & jdbc event handler
 export STATE_STORE_JDBC_PWD=123456
+export EVENT_HANDLER_JDBC_PWD=123456
 
 java -cp flink-autoscaler-standalone-{{< version >}}.jar:${JDBC_DRIVER_JAR} \
 org.apache.flink.autoscaler.standalone.StandaloneAutoscalerEntrypoint \
 --autoscaler.standalone.fetcher.flink-cluster.host localhost \
 --autoscaler.standalone.fetcher.flink-cluster.port 8081 \
 --autoscaler.standalone.state-store.type jdbc \
 --autoscaler.standalone.state-store.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
---autoscaler.standalone.state-store.jdbc.username root
+--autoscaler.standalone.state-store.jdbc.username root \
+--autoscaler.standalone.event-handler.type jdbc \
+--autoscaler.standalone.event-handler.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
+--autoscaler.standalone.event-handler.jdbc.username root

Review Comment:
   I'm still working on this PR now, I found it's better to include this PR in `1.8.0`. Because I believe  jdbcStateStore and jdbcEventHandler  always work together.
   
   During I test the `jdbcEventHandler`, I found some defects of `jdbcStateStore`, such as: FLINK-34389, this comment https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1480012807. 
   
   It means if we includes `jdbcEventHandler` in  this version we can ensure they work well together. 
   
   So I'd like to increase this priority, WDYT?
   
   Note: the prod code is done(you can help review if you are free), and the progress of unit test and ITCase is about 40% and I will finish it asap..



-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1490288389


##########
docs/content/docs/custom-resource/autoscaler.md:
##########
@@ -286,16 +286,20 @@ please download JDBC driver and initialize database and table first.
 
 ```
 JDBC_DRIVER_JAR=./mysql-connector-java-8.0.30.jar
-# export the password of jdbc state store
+# export the password of jdbc state store & jdbc event handler
 export STATE_STORE_JDBC_PWD=123456
+export EVENT_HANDLER_JDBC_PWD=123456
 
 java -cp flink-autoscaler-standalone-{{< version >}}.jar:${JDBC_DRIVER_JAR} \
 org.apache.flink.autoscaler.standalone.StandaloneAutoscalerEntrypoint \
 --autoscaler.standalone.fetcher.flink-cluster.host localhost \
 --autoscaler.standalone.fetcher.flink-cluster.port 8081 \
 --autoscaler.standalone.state-store.type jdbc \
 --autoscaler.standalone.state-store.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
---autoscaler.standalone.state-store.jdbc.username root
+--autoscaler.standalone.state-store.jdbc.username root \
+--autoscaler.standalone.event-handler.type jdbc \
+--autoscaler.standalone.event-handler.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
+--autoscaler.standalone.event-handler.jdbc.username root

Review Comment:
   Thanks for the feedback! I have updated.



-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#issuecomment-1953567662

   Hi @mxm  @gyfora , gentle ping, do you have any other comments?


-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1480012807


##########
docs/content/docs/custom-resource/autoscaler.md:
##########
@@ -286,16 +286,20 @@ please download JDBC driver and initialize database and table first.
 
 ```
 JDBC_DRIVER_JAR=./mysql-connector-java-8.0.30.jar
-# export the password of jdbc state store
+# export the password of jdbc state store & jdbc event handler
 export STATE_STORE_JDBC_PWD=123456
+export EVENT_HANDLER_JDBC_PWD=123456
 
 java -cp flink-autoscaler-standalone-{{< version >}}.jar:${JDBC_DRIVER_JAR} \
 org.apache.flink.autoscaler.standalone.StandaloneAutoscalerEntrypoint \
 --autoscaler.standalone.fetcher.flink-cluster.host localhost \
 --autoscaler.standalone.fetcher.flink-cluster.port 8081 \
 --autoscaler.standalone.state-store.type jdbc \
 --autoscaler.standalone.state-store.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
---autoscaler.standalone.state-store.jdbc.username root
+--autoscaler.standalone.state-store.jdbc.username root \
+--autoscaler.standalone.event-handler.type jdbc \
+--autoscaler.standalone.event-handler.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
+--autoscaler.standalone.event-handler.jdbc.username root

Review Comment:
   These database related options are similar between jdbcStateStore and jdbcEventHandler. I'm not sure whether we should merge them or stay as is.
   
   ## Solution1: Merging them 
   
   Merge them means we rename `autoscaler.standalone.state-store.jdbc.username` to `autoscaler.standalone.jdbc-plugin.username`. And both of jdbcStateStore and jdbcEventHandler will use `autoscaler.standalone.jdbc-plugin.username` and `autoscaler.standalone.jdbc-plugin.url`.
   
   ## Solution2: Independent options
   
   Defining`autoscaler.standalone.event-handler.jdbc.url` and `autoscaler.standalone.state-store.jdbc.url`, it means jdbcStateStore and jdbcEventHandler can use different databases and different users.
   
   I believe solution1 is pretty easy to  use, and it's enough for most of users.
   
   WDYT? @gyfora  @mxm 



-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1480045403


##########
docs/content/docs/custom-resource/autoscaler.md:
##########
@@ -286,16 +286,20 @@ please download JDBC driver and initialize database and table first.
 
 ```
 JDBC_DRIVER_JAR=./mysql-connector-java-8.0.30.jar
-# export the password of jdbc state store
+# export the password of jdbc state store & jdbc event handler
 export STATE_STORE_JDBC_PWD=123456
+export EVENT_HANDLER_JDBC_PWD=123456
 
 java -cp flink-autoscaler-standalone-{{< version >}}.jar:${JDBC_DRIVER_JAR} \
 org.apache.flink.autoscaler.standalone.StandaloneAutoscalerEntrypoint \
 --autoscaler.standalone.fetcher.flink-cluster.host localhost \
 --autoscaler.standalone.fetcher.flink-cluster.port 8081 \
 --autoscaler.standalone.state-store.type jdbc \
 --autoscaler.standalone.state-store.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
---autoscaler.standalone.state-store.jdbc.username root
+--autoscaler.standalone.state-store.jdbc.username root \
+--autoscaler.standalone.event-handler.type jdbc \
+--autoscaler.standalone.event-handler.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
+--autoscaler.standalone.event-handler.jdbc.username root

Review Comment:
   I'm still working on this PR now, I found it's better to include this PR in `1.8.0`. Because I believe  jdbcStateStore and jdbcEventHandler  always work together.
   
   During I test the `jdbcEventHandler`, I found some defects of `jdbcStateStore`, such as: FLINK-34389, this comment https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1480012807. 
   
   It means if we includes `jdbcEventHandler` in  this version we can ensure they work well together. If we release the jdbcStateStore in this version, and release jdbcEventHandler in the next version. We might need to change the jdbcStateStore again in the next version. It's better to release a final version for users, and it won't introduce any incompatibility problem.
   
   So I'd like to increase this priority, WDYT?
   
   Note: the prod code is done(you can help review if you are free), and the progress of unit test and ITCase is about 40% and I will finish it asap..



-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "mxm (via GitHub)" <gi...@apache.org>.
mxm commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1490807423


##########
flink-autoscaler-standalone/src/main/java/org/apache/flink/autoscaler/standalone/config/AutoscalerStandaloneOptions.java:
##########
@@ -109,4 +110,51 @@ private static ConfigOptions.OptionBuilder autoscalerStandaloneConfig(String key
                                                     + "export the password using this environment variable.",
                                             code(STATE_STORE_TYPE.key()), code(JDBC.toString()))
                                     .build());
+
+    public static final ConfigOption<EventHandlerType> EVENT_HANDLER_TYPE =
+            autoscalerStandaloneConfig("event-handler.type")
+                    .enumType(EventHandlerType.class)
+                    .defaultValue(EventHandlerType.LOGGING)
+                    .withDescription("The autoscaler event handler type.");

Review Comment:
   Got it. That makes sense. I meant what if I want to print both to the console and store in JDBC? Currently there can only be one event handler active at a 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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [FLINK-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1490275258


##########
flink-autoscaler-standalone/src/main/java/org/apache/flink/autoscaler/standalone/config/AutoscalerStandaloneOptions.java:
##########
@@ -109,4 +110,51 @@ private static ConfigOptions.OptionBuilder autoscalerStandaloneConfig(String key
                                                     + "export the password using this environment variable.",
                                             code(STATE_STORE_TYPE.key()), code(JDBC.toString()))
                                     .build());
+
+    public static final ConfigOption<EventHandlerType> EVENT_HANDLER_TYPE =
+            autoscalerStandaloneConfig("event-handler.type")
+                    .enumType(EventHandlerType.class)
+                    .defaultValue(EventHandlerType.LOGGING)
+                    .withDescription("The autoscaler event handler type.");

Review Comment:
   We can configure them separately.
   
   ```
   --autoscaler.standalone.state-store.type jdbc \
   --autoscaler.standalone.event-handler.type jdbc \
   ```
   
   After users choose JDBC for them, we can reuse all jdbc related options, such as:
   
   ```
   autoscaler.standalone.jdbc.username
   autoscaler.standalone.jdbc.url
   ```
   



-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1490909393


##########
flink-autoscaler-standalone/src/main/java/org/apache/flink/autoscaler/standalone/config/AutoscalerStandaloneOptions.java:
##########
@@ -109,4 +110,51 @@ private static ConfigOptions.OptionBuilder autoscalerStandaloneConfig(String key
                                                     + "export the password using this environment variable.",
                                             code(STATE_STORE_TYPE.key()), code(JDBC.toString()))
                                     .build());
+
+    public static final ConfigOption<EventHandlerType> EVENT_HANDLER_TYPE =
+            autoscalerStandaloneConfig("event-handler.type")
+                    .enumType(EventHandlerType.class)
+                    .defaultValue(EventHandlerType.LOGGING)
+                    .withDescription("The autoscaler event handler type.");

Review Comment:
   Sorry, I misunderstand your question before, thanks for your clarification!
   
   > I meant what if I want to print both to the console and store in JDBC? Currently there can only be one event handler active at a time.
   
   You are right, currently there are only be one event handler or state store active at a time. I see kubernetes operator is similar, kubernetes operator only creates one event handler or state store.
   
   If users want to use multiple event handlers, I can implement `CompositeEventHandler` in the future. Users can configure:
   
   ```
   --autoscaler.standalone.event-handler.type composite
   --autoscaler.standalone.event-handler.composite.types jdbc,logging
   ```
   
   The `CompositeStateStore` is similar to `CompositeEventHandler`:
   
   ```
   --autoscaler.standalone.state-store.type composite
   --autoscaler.standalone.state-store.composite.types jdbc,memory
   ```
   
   In the short term, it seems unnecessary. I can implement them in the future if some users feedback it's useful in some scenarios. What do you think about the solution and time plan?
   



-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "mxm (via GitHub)" <gi...@apache.org>.
mxm commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1491077036


##########
flink-autoscaler-standalone/src/main/java/org/apache/flink/autoscaler/standalone/config/AutoscalerStandaloneOptions.java:
##########
@@ -109,4 +110,51 @@ private static ConfigOptions.OptionBuilder autoscalerStandaloneConfig(String key
                                                     + "export the password using this environment variable.",
                                             code(STATE_STORE_TYPE.key()), code(JDBC.toString()))
                                     .build());
+
+    public static final ConfigOption<EventHandlerType> EVENT_HANDLER_TYPE =
+            autoscalerStandaloneConfig("event-handler.type")
+                    .enumType(EventHandlerType.class)
+                    .defaultValue(EventHandlerType.LOGGING)
+                    .withDescription("The autoscaler event handler type.");

Review Comment:
   That sounds good to me. We can add the composite handler if necessary in the 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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui merged PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765


-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#discussion_r1480012807


##########
docs/content/docs/custom-resource/autoscaler.md:
##########
@@ -286,16 +286,20 @@ please download JDBC driver and initialize database and table first.
 
 ```
 JDBC_DRIVER_JAR=./mysql-connector-java-8.0.30.jar
-# export the password of jdbc state store
+# export the password of jdbc state store & jdbc event handler
 export STATE_STORE_JDBC_PWD=123456
+export EVENT_HANDLER_JDBC_PWD=123456
 
 java -cp flink-autoscaler-standalone-{{< version >}}.jar:${JDBC_DRIVER_JAR} \
 org.apache.flink.autoscaler.standalone.StandaloneAutoscalerEntrypoint \
 --autoscaler.standalone.fetcher.flink-cluster.host localhost \
 --autoscaler.standalone.fetcher.flink-cluster.port 8081 \
 --autoscaler.standalone.state-store.type jdbc \
 --autoscaler.standalone.state-store.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
---autoscaler.standalone.state-store.jdbc.username root
+--autoscaler.standalone.state-store.jdbc.username root \
+--autoscaler.standalone.event-handler.type jdbc \
+--autoscaler.standalone.event-handler.jdbc.url jdbc:mysql://localhost:3306/flink_autoscaler \
+--autoscaler.standalone.event-handler.jdbc.username root

Review Comment:
   These database related options are similar between jdbcStateStore and jdbcEventHandler. I'm not sure whether we should merge them or stay as is.
   
   Merge them means we rename `autoscaler.standalone.state-store.jdbc.username` to `autoscaler.standalone.jdbc-plugin.username`. And both of jdbcStateStore and jdbcEventHandler will use `autoscaler.standalone.jdbc-plugin.username` and `autoscaler.standalone.jdbc-plugin.url`.
   
   If  we define `autoscaler.standalone.event-handler.jdbc.url` and `autoscaler.standalone.state-store.jdbc.url`, it means jdbcStateStore and jdbcEventHandler can use different databases and different users.
   
   WDYT? @gyfora  @mxm 



-- 
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-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#issuecomment-1929242059

   Sounds like @1996fanrui wants to have a peaceful vacation. I don't see any reason why we should rush this into the release or delay the release because of 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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [FLINK-33636][autoscaler] Support the JDBCAutoScalerEventHandler [flink-kubernetes-operator]

Posted by "mxm (via GitHub)" <gi...@apache.org>.
mxm commented on PR #765:
URL: https://github.com/apache/flink-kubernetes-operator/pull/765#issuecomment-1929239360

   @1996fanrui How important is it for you to get this into 1.8.0? I think the 1.8.0 release could wait until then.


-- 
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