You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "JunRuiLee (via GitHub)" <gi...@apache.org> on 2023/12/01 10:03:03 UTC
[PR] [FLINK-33576][core] Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax. [flink]
JunRuiLee opened a new pull request, #23852:
URL: https://github.com/apache/flink/pull/23852
<!--
*Thank you very much for contributing to Apache Flink - we are happy that you want to help us improve Flink. To help the community review your contribution in the best possible way, please go through the checklist below, which will get the contribution into a shape in which it can be best reviewed.*
*Please understand that we do not do this to make contributions to Flink a hassle. In order to uphold a high standard of quality for code contributions, while at the same time managing a large number of contributions, we need contributors to prepare the contributions well, and give reviewers enough contextual information for the review. Please also understand that contributions that do not follow this guide will take longer to review and thus typically be picked up with lower priority by the community.*
## Contribution Checklist
- Make sure that the pull request corresponds to a [JIRA issue](https://issues.apache.org/jira/projects/FLINK/issues). Exceptions are made for typos in JavaDoc or documentation files, which need no JIRA issue.
- Name the pull request in the form "[FLINK-XXXX] [component] Title of the pull request", where *FLINK-XXXX* should be replaced by the actual issue number. Skip *component* if you are unsure about which is the best component.
Typo fixes that have no associated JIRA issue should be named following this pattern: `[hotfix] [docs] Fix typo in event time introduction` or `[hotfix] [javadocs] Expand JavaDoc for PuncuatedWatermarkGenerator`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Make sure that the change passes the automated tests, i.e., `mvn clean verify` passes. You can set up Azure Pipelines CI to do that following [this guide](https://cwiki.apache.org/confluence/display/FLINK/Azure+Pipelines#AzurePipelines-Tutorial:SettingupAzurePipelinesforaforkoftheFlinkrepository).
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message (including the JIRA id)
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
## What is the purpose of the change
Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax.
## Brief change log
- *Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax.*
- *Change the UT case to use new conf file.*
## Verifying this change
This change is already covered by existing tests.
## 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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33576][core] Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax. [flink]
Posted by "JunRuiLee (via GitHub)" <gi...@apache.org>.
JunRuiLee commented on PR #23852:
URL: https://github.com/apache/flink/pull/23852#issuecomment-1886692234
Hi @zhuzhurk , Thanks for reviewing. I've updated the PR with the necessary changes. Please take a look~
Here are some clarifications I'd like to make:
1. Concerning the thread safety issues of the GlobalConfiguration.isStandardYaml state during concurrent tests, the current testing framework in Flink ensures tests are run sequentially between different test classes within the same process. This setup means we only need to ensure the consistency of that state upon the completion of each test class.
2. Regarding the updates needed for comments within the code and specific code references to "flink-conf.yaml" (for example, in YarnTestBase), I intend to address these adjustments in a follow-up task. This subsequent task will involve updating the default configuration file in flink-dist to config.yaml, at which time I plan to collectively revise both the comments in the code and the relevant code references.
--
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-33576][core] Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax. [flink]
Posted by "JunRuiLee (via GitHub)" <gi...@apache.org>.
JunRuiLee commented on code in PR #23852:
URL: https://github.com/apache/flink/pull/23852#discussion_r1444659693
##########
flink-end-to-end-tests/flink-end-to-end-tests-common/src/main/java/org/apache/flink/tests/util/flink/FlinkDistribution.java:
##########
@@ -422,7 +422,10 @@ public void appendConfiguration(Configuration config) throws IOException {
.map(entry -> entry.getKey() + ": " + entry.getValue())
.collect(Collectors.toList());
- Files.write(conf.resolve("flink-conf.yaml"), configurationLines);
+ // NOTE: Before we change the default conf file in the flink-dist to 'config.yaml', we
+ // need to use the legacy flink conf file 'flink-conf.yaml' here.
Review Comment:
Yes
--
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-33576][core] Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax. [flink]
Posted by "zhuzhurk (via GitHub)" <gi...@apache.org>.
zhuzhurk closed pull request #23852: [FLINK-33576][core] Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax.
URL: https://github.com/apache/flink/pull/23852
--
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-33576][core] Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax. [flink]
Posted by "JunRuiLee (via GitHub)" <gi...@apache.org>.
JunRuiLee commented on PR #23852:
URL: https://github.com/apache/flink/pull/23852#issuecomment-1886989525
@zhuzhurk 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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33576][core] Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax. [flink]
Posted by "JunRuiLee (via GitHub)" <gi...@apache.org>.
JunRuiLee commented on PR #23852:
URL: https://github.com/apache/flink/pull/23852#issuecomment-1888377286
@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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33576][core] Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax. [flink]
Posted by "zhuzhurk (via GitHub)" <gi...@apache.org>.
zhuzhurk commented on code in PR #23852:
URL: https://github.com/apache/flink/pull/23852#discussion_r1448706412
##########
flink-core/src/main/java/org/apache/flink/configuration/GlobalConfiguration.java:
##########
@@ -241,6 +265,95 @@ private static Configuration loadYAMLResource(File file) {
return config;
}
+ /**
+ * Flattens a nested configuration map to be only one level deep.
+ *
+ * <p>Nested keys are concatinated using the {@code KEY_SEPARATOR} character. So that:
+ *
+ * <pre>
+ * keyA:
+ * keyB:
+ * keyC: "hello"
+ * keyD: "world"
+ * </pre>
+ *
+ * <p>becomes:
+ *
+ * <pre>
+ * keyA.keyB.keyC: "hello"
+ * keyA.keyB.keyD: "world"
+ * </pre>
+ *
+ * @param config an arbitrarily nested config map
+ * @param keyPrefix The string to prefix the keys in the current config level
+ * @return A flattened, 1 level deep map
+ */
+ @SuppressWarnings("unchecked")
+ private static Map<String, Object> flatten(
+ @Nonnull Map<String, Object> config, String keyPrefix) {
Review Comment:
Flink parameters are by default non-null. Only nullable ones should be explicitly annotated with `@Nullable`.
--
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-33576][core] Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax. [flink]
Posted by "zhuzhurk (via GitHub)" <gi...@apache.org>.
zhuzhurk commented on code in PR #23852:
URL: https://github.com/apache/flink/pull/23852#discussion_r1444574392
##########
flink-core/src/test/java/org/apache/flink/configuration/GlobalConfigurationTest.java:
##########
@@ -40,7 +46,7 @@ class GlobalConfigurationTest {
@Test
void testConfigurationYAML() {
Review Comment:
Maybe rename it to `testConfigurationWithLegacyYAML`
##########
flink-core/src/main/java/org/apache/flink/configuration/GlobalConfiguration.java:
##########
@@ -257,7 +371,19 @@ public static boolean isSensitive(String key) {
return false;
}
+ public static String getFlinkConfFilename() {
+ if (isStandardYaml()) {
+ return FLINK_CONF_FILENAME;
+ } else {
+ return LEGACY_FLINK_CONF_FILENAME;
+ }
+ }
+
public static boolean isStandardYaml() {
return standardYaml;
}
+
+ public static void setStandardYaml(boolean standardYaml) {
Review Comment:
Should be marked as `@VisibleForTesting`.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/clusterframework/BootstrapToolsTest.java:
##########
@@ -654,7 +653,13 @@ void testGetEnvironmentVariablesErroneous() {
}
@Test
- void testWriteConfigurationAndReload() throws IOException {
+ void testWriteConfigurationAndReload() throws Exception {
+ testWriteConfigurationAndReloadInternal(false);
+ testWriteConfigurationAndReloadInternal(true);
+ }
+
+ private void testWriteConfigurationAndReloadInternal(boolean standardYaml) throws IOException {
+ GlobalConfiguration.setStandardYaml(standardYaml);
Review Comment:
With this kind of changes, the tests can be unstable when running concurrently.
##########
flink-core/src/main/java/org/apache/flink/configuration/ConfigurationUtils.java:
##########
@@ -112,25 +112,26 @@ public static String[] parseLocalStateDirectories(Configuration configuration) {
}
/**
- * Parses a string as a map of strings. The expected format of the map is:
+ * Parses a string as a map of strings. The expected format of the map which parsed by FLINK
Review Comment:
which parsed -> to be parsed
##########
flink-core/src/main/java/org/apache/flink/configuration/GlobalConfiguration.java:
##########
@@ -241,6 +263,98 @@ private static Configuration loadYAMLResource(File file) {
return config;
}
+ /**
+ * Flattens a nested configuration map to be only one level deep.
+ *
+ * <p>Nested keys are concatinated using the {@code KEY_SEPARATOR} character. So that:
+ *
+ * <pre>
+ * keyA:
+ * keyB:
+ * keyC: "hello"
+ * keyD: "world"
+ * </pre>
+ *
+ * <p>becomes:
+ *
+ * <pre>
+ * keyA.keyB.keyC: "hello"
+ * keyA.keyB.keyD: "world"
+ * </pre>
+ *
+ * @param config an arbitrarily nested config map
+ * @param keyPrefix The string to prefix the keys in the current config level
+ * @return A flattened, 1 level deep map
+ */
+ @SuppressWarnings("unchecked")
+ private static Map<String, Object> flatten(Map<String, Object> config, String keyPrefix) {
+ final Map<String, Object> flattenedMap = new HashMap<>();
+
+ if (config == null) {
Review Comment:
Is this parameter nullable?
##########
flink-end-to-end-tests/flink-end-to-end-tests-common/src/main/java/org/apache/flink/tests/util/flink/FlinkDistribution.java:
##########
@@ -422,7 +422,10 @@ public void appendConfiguration(Configuration config) throws IOException {
.map(entry -> entry.getKey() + ": " + entry.getValue())
.collect(Collectors.toList());
- Files.write(conf.resolve("flink-conf.yaml"), configurationLines);
+ // NOTE: Before we change the default conf file in the flink-dist to 'config.yaml', we
+ // need to use the legacy flink conf file 'flink-conf.yaml' here.
Review Comment:
IIUC, we can do this before Flink 2.0, after we have reworked the E2E tests?
##########
flink-core/src/test/java/org/apache/flink/configuration/GlobalConfigurationTest.java:
##########
@@ -112,15 +171,37 @@ void testInvalidConfiguration() {
}
@Test
- // We allow malformed YAML files
+ // We allow malformed YAML files if loaded legacy flink conf
void testInvalidYamlFile() throws IOException {
- final File confFile = new File(tmpDir.getPath(), GlobalConfiguration.FLINK_CONF_FILENAME);
+ final File confFile =
+ new File(tmpDir.getPath(), GlobalConfiguration.LEGACY_FLINK_CONF_FILENAME);
try (PrintWriter pw = new PrintWriter(confFile)) {
pw.append("invalid");
}
assertThat(GlobalConfiguration.loadConfiguration(tmpDir.getAbsolutePath())).isNotNull();
+ // Clear the standard yaml flag to avoid has impact to other cases.
+ GlobalConfiguration.setStandardYaml(true);
+ }
+
+ @Test
+ // We do not allow malformed YAML files if loaded standard yaml
+ void testInvalidYamlFile_StandardYaml() throws IOException {
Review Comment:
`_` should be avoided in method names
##########
flink-core/src/test/java/org/apache/flink/configuration/GlobalConfigurationTest.java:
##########
@@ -112,15 +171,37 @@ void testInvalidConfiguration() {
}
@Test
- // We allow malformed YAML files
+ // We allow malformed YAML files if loaded legacy flink conf
void testInvalidYamlFile() throws IOException {
Review Comment:
Maybe rename it to `testInvalidLegacyYamlFile`
##########
flink-core/src/test/java/org/apache/flink/configuration/GlobalConfigurationTest.java:
##########
@@ -85,11 +91,64 @@ void testConfigurationYAML() {
assertThat(conf.getString("mykey8", "null")).isEqualTo("null");
assertThat(conf.getString("mykey9", null)).isEqualTo("myvalue10");
} finally {
+ // Clear the standard yaml flag to avoid has impact to other cases.
Review Comment:
avoid has impact -> avoid impact
##########
flink-core/src/main/java/org/apache/flink/configuration/ConfigurationUtils.java:
##########
@@ -112,25 +112,26 @@ public static String[] parseLocalStateDirectories(Configuration configuration) {
}
/**
- * Parses a string as a map of strings. The expected format of the map is:
+ * Parses a string as a map of strings. The expected format of the map which parsed by FLINK
+ * parser is:
*
* <pre>
* key1:value1,key2:value2
* </pre>
*
+ * <p>The expected format of the map which parsed by standard YAML parser is:
Review Comment:
which parsed -> to be parsed
##########
flink-runtime/src/test/java/org/apache/flink/runtime/clusterframework/BootstrapToolsTest.java:
##########
@@ -654,7 +653,13 @@ void testGetEnvironmentVariablesErroneous() {
}
@Test
- void testWriteConfigurationAndReload() throws IOException {
+ void testWriteConfigurationAndReload() throws Exception {
+ testWriteConfigurationAndReloadInternal(false);
+ testWriteConfigurationAndReloadInternal(true);
Review Comment:
It's better to have two test methods to verify them separately.
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/plan/batch/sql/RowLevelUpdateTest.java:
##########
@@ -136,7 +136,7 @@ void testUpdateWithCustomColumns() {
"CREATE TABLE t (a int, b string, c double) WITH"
+ " ("
+ "'connector' = 'test-update-delete', "
- + "'required-columns-for-update' = 'b;c', "
+ + "'required-columns-for-update' = '[b, c]', "
Review Comment:
The incompatibility of existing SQLs can be a problem.
I think we need to find way that allows users to use standard YAML config file without changing the existing sql jobs.
--
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-33576][core] Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax. [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #23852:
URL: https://github.com/apache/flink/pull/23852#issuecomment-1835818643
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "196ed86c68a271a1357d172ee23059930b0cfa0d",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "196ed86c68a271a1357d172ee23059930b0cfa0d",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 196ed86c68a271a1357d172ee23059930b0cfa0d 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-33576][core] Introduce new Flink conf file 'config.yaml' supporting standard YAML syntax. [flink]
Posted by "JunRuiLee (via GitHub)" <gi...@apache.org>.
JunRuiLee commented on PR #23852:
URL: https://github.com/apache/flink/pull/23852#issuecomment-1891254763
@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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org