Re: [PR] [FLINK-33577][dist] Change the default config file to config.yaml in flink-dist. [flink]

2024-01-25 Thread via GitHub 


zhuzhurk closed pull request #24177: [FLINK-33577][dist] Change the default 
config file to config.yaml in flink-dist.
URL: https://github.com/apache/flink/pull/24177


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

Re: [PR] [FLINK-33577][dist] Change the default config file to config.yaml in flink-dist. [flink]

2024-01-25 Thread via GitHub 


zhuzhurk commented on PR #24177:
URL: https://github.com/apache/flink/pull/24177#issuecomment-1910452867

   The CI gives green. I will squash and merge the commits locally.


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

Re: [PR] [FLINK-33577][dist] Change the default config file to config.yaml in flink-dist. [flink]

2024-01-25 Thread via GitHub 


JunRuiLee commented on PR #24177:
URL: https://github.com/apache/flink/pull/24177#issuecomment-1910014658

   Thanks @HuangXingBo for reviews, I've updated this pr follow your 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

Re: [PR] [FLINK-33577][dist] Change the default config file to config.yaml in flink-dist. [flink]

2024-01-24 Thread via GitHub 


zhuzhurk commented on code in PR #24177:
URL: https://github.com/apache/flink/pull/24177#discussion_r1465900132


##
flink-end-to-end-tests/test-scripts/common.sh:
##
@@ -56,6 +56,8 @@ if [[ -z "${FLINK_CONF_DIR:-}" ]]; then
 FLINK_CONF_DIR="$FLINK_DIR/conf"
 fi
 FLINK_CONF=${FLINK_CONF_DIR}/config.yaml
+# Flatten the configuration file config.yaml to enable end-to-end test cases 
can modify 

Review Comment:
   can -> which will



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

Re: [PR] [FLINK-33577][dist] Change the default config file to config.yaml in flink-dist. [flink]

2024-01-24 Thread via GitHub 


HuangXingBo commented on code in PR #24177:
URL: https://github.com/apache/flink/pull/24177#discussion_r1465791701


##
flink-python/pyflink/pyflink_gateway_server.py:
##
@@ -43,8 +43,24 @@ def on_windows():
 return platform.system() == "Windows"
 
 
-def read_from_config(key, default_value, flink_conf_file):
+def read_from_config(key, default_value, flink_conf_directory):
+import yaml

Review Comment:
   I feel the logic of this block of code can be refactored:
   ```
   if  flink-conf.yaml exist:
   yaml.parse
   elif conf.yaml exist:
   old parse logic
   else
   return default value
   ```



##
flink-python/pyflink/table/table_environment.py:
##
@@ -1542,21 +1542,55 @@ def _set_python_executable_for_local_executor(self):
 def _add_jars_to_j_env_config(self, config_key):
 jvm = get_gateway().jvm
 jar_urls = self.get_config().get(config_key, None)
+
+isStandardYaml = jvm.org.apache.flink.configuration. \

Review Comment:
   `isStandardYaml->is_standard_yaml`



##
flink-python/pyflink/table/table_environment.py:
##
@@ -1542,21 +1542,55 @@ def _set_python_executable_for_local_executor(self):
 def _add_jars_to_j_env_config(self, config_key):
 jvm = get_gateway().jvm
 jar_urls = self.get_config().get(config_key, None)
+
+isStandardYaml = jvm.org.apache.flink.configuration. \
+GlobalConfiguration.isStandardYaml()
 if jar_urls is not None:
-# normalize
 jar_urls_list = []
-for url in jar_urls.split(";"):
-url = url.strip()
-if url != "":
-jar_urls_list.append(jvm.java.net.URL(url).toString())
-j_configuration = get_j_env_configuration(self._get_j_env())
-if j_configuration.containsKey(config_key):
-for url in j_configuration.getString(config_key, 
"").split(";"):
-url = url.strip()
-if url != "" and url not in jar_urls_list:
-jar_urls_list.append(url)
+# normalize
+if not isStandardYaml:
+self._parse_url_by_legacy_parser(jar_urls, jar_urls_list, jvm)
+j_configuration = get_j_env_configuration(self._get_j_env())
+self._parse_j_env_url_by_legacy_parser(config_key, 
j_configuration, jar_urls_list)
+else:
+import yaml
+parsed_jar_urls = yaml.safe_load(jar_urls)
+if isinstance(parsed_jar_urls, list):
+for url in parsed_jar_urls:
+url = url.strip()
+if url != "":
+
jar_urls_list.append(jvm.java.net.URL(url).toString())
+else:
+self._parse_url_by_legacy_parser(jar_urls, jar_urls_list, 
jvm)
+
+j_configuration = get_j_env_configuration(self._get_j_env())
+if j_configuration.containsKey(config_key):
+jar_urls_from_j_env = yaml. \
+safe_load(j_configuration.getString(config_key, ""))
+if isinstance(jar_urls_from_j_env, list):
+for url in jar_urls_from_j_env:
+url = url.strip()
+if url != "" and url not in jar_urls_list:
+jar_urls_list.append(url)
+else:
+self._parse_j_env_url_by_legacy_parser(
+config_key, j_configuration, jar_urls_list)
+
 j_configuration.setString(config_key, ";".join(jar_urls_list))
 
+def _parse_j_env_url_by_legacy_parser(self, config_key, j_configuration, 
jar_urls_list):
+if j_configuration.containsKey(config_key):
+for url in j_configuration.getString(config_key, "").split(";"):
+url = url.strip()
+if url != "" and url not in jar_urls_list:
+jar_urls_list.append(url)
+
+def _parse_url_by_legacy_parser(self, jar_urls, jar_urls_list, jvm):
+for url in jar_urls.split(";"):
+url = url.strip()
+if url != "":
+jar_urls_list.append(jvm.java.net.URL(url).toString())
+

Review Comment:
   I feel the logic of this block of code can be refactored:
   ```
   def _add_jars_to_j_env_config(self, config_key):
jar_urls = self.get_config().get(config_key, None)
   
if jar_urls:
jvm = get_gateway().jvm
jar_urls_list = []
_parse_urls([jvm.java.net.URL(url).toString() if item else "" 
for item in parse_jars_value(jar_urls, jvm)], jar_urls_list)
   
 j_configuration = 
get_j_env_configuration(self._get_j_env())
 
_parse_urls(parse_jars_value(j_configuration.getString(config_key, ""), jvm), 
jar_urls_list)

Re: [PR] [FLINK-33577][dist] Change the default config file to config.yaml in flink-dist. [flink]

2024-01-24 Thread via GitHub 


JunRuiLee commented on PR #24177:
URL: https://github.com/apache/flink/pull/24177#issuecomment-1909296304

   Thanks @zhuzhurk for reviews, I've updated this pr accordingly, 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

Re: [PR] [FLINK-33577][dist] Change the default config file to config.yaml in flink-dist. [flink]

2024-01-24 Thread via GitHub 


zhuzhurk commented on code in PR #24177:
URL: https://github.com/apache/flink/pull/24177#discussion_r1464880838


##
flink-end-to-end-tests/test-scripts/common_docker.sh:
##
@@ -48,7 +48,8 @@ function build_image() {
 local server_pid=$!
 
 echo "Preparing Dockeriles"
-retry_times_with_exponential_backoff 5 git clone 
https://github.com/apache/flink-docker.git --branch dev-master --single-branch
+# TODO only for test, after FLINK-34205 we should revert this change
+retry_times_with_exponential_backoff 5 git clone 
https://github.com/JunRuiLee/flink-docker.git --branch using_bash_java_utils 
--single-branch

Review Comment:
   The change needs to be updated.



##
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java:
##
@@ -41,7 +41,6 @@
 import java.io.FileWriter;
 import java.io.IOException;
 import java.io.PrintWriter;
-import java.util.Map;
 import java.util.stream.Stream;

Review Comment:
   This hotfix is already done in flink master.



##
flink-end-to-end-tests/test-scripts/common.sh:
##
@@ -50,6 +50,14 @@ TEST_INFRA_DIR=`pwd -P`
 cd $TEST_ROOT
 
 source "${TEST_INFRA_DIR}/common_utils.sh"
+source "${FLINK_DIR}/bin/bash-java-utils.sh"
+
+if [[ -z "${FLINK_CONF_DIR:-}" ]]; then
+FLINK_CONF_DIR="$FLINK_DIR/conf"
+fi
+FLINK_CONF=${FLINK_CONF_DIR}/config.yaml
+output=$(updateAndGetFlinkConfiguration "${FLINK_CONF_DIR}" "${FLINK_DIR}/bin" 
"${FLINK_DIR}/lib" -flatten)

Review Comment:
   Maybe add a comment here to state that a flattened config file is required 
because many cases will modify the config file.



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

Re: [PR] [FLINK-33577][dist] Change the default config file to config.yaml in flink-dist. [flink]

2024-01-23 Thread via GitHub 


JunRuiLee commented on PR #24177:
URL: https://github.com/apache/flink/pull/24177#issuecomment-1907348848

   Hi @HuangXingBo , this pr includes some PyFlink changes related to the 
adoption of a new configuration file, config.yaml. Could you please help review 
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

Re: [PR] [FLINK-33577][dist] Change the default config file to config.yaml in flink-dist. [flink]

2024-01-23 Thread via GitHub 


flinkbot commented on PR #24177:
URL: https://github.com/apache/flink/pull/24177#issuecomment-1906045313

   
   ## CI report:
   
   * e9748afaeb94ceac244d233c00f8b042a0edaa95 UNKNOWN
   
   
   Bot commands
 The @flinkbot bot supports the following commands:
   
- `@flinkbot run azure` re-run the last Azure build
   


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

[PR] [FLINK-33577][dist] Change the default config file to config.yaml in flink-dist. [flink]

2024-01-23 Thread via GitHub 


JunRuiLee opened a new pull request, #24177:
URL: https://github.com/apache/flink/pull/24177

   
   
   
   ## What is the purpose of the change
   
   Change the default config file to config.yaml in flink-dist.
   
   ## Brief change log
   
   
 - Change the default config file to config.yaml in flink-dist.
 - Update the content 'flink-conf.yaml' in the code to use 'config.yaml'
   
   
   ## Verifying this change
   
   This change is already covered by existing tests, include all e2e cases and 
it cases.
   
   ## Does this pull request potentially affect one of the following parts:
   
 - Dependencies (does it add or upgrade a dependency): (yes / **no**)
 - The public API, i.e., is any changed class annotated with 
`@Public(Evolving)`: (yes / **no**)
 - The serializers: (yes / **no** / don't know)
 - The runtime per-record code paths (performance sensitive): (yes / **no** 
/ don't know)
 - Anything that affects deployment or recovery: JobManager (and its 
components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / **no** / don't 
know)
 - The S3 file system connector: (yes / **no** / don't know)
   
   ## Documentation
   
 - Does this pull request introduce a new feature? (**yes** / no)
 - If yes, how is the feature documented? (not applicable / docs / JavaDocs 
/ **not documented**)
   


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