Re: [PR] [FLINK-34080][configuration] Simplify the Configuration [flink]
1996fanrui merged PR #24088: URL: https://github.com/apache/flink/pull/24088 -- 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-34080][configuration] Simplify the Configuration [flink]
1996fanrui commented on PR #24088: URL: https://github.com/apache/flink/pull/24088#issuecomment-1899820154 Thanks @Sxnan for the reviewing! The CI is green, merging~ -- 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-34080][configuration] Simplify the Configuration [flink]
1996fanrui commented on PR #24088: URL: https://github.com/apache/flink/pull/24088#issuecomment-1899532157 @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
Re: [PR] [FLINK-34080][configuration] Simplify the Configuration [flink]
1996fanrui commented on PR #24088: URL: https://github.com/apache/flink/pull/24088#issuecomment-1897658106 @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
Re: [PR] [FLINK-34080][configuration] Simplify the Configuration [flink]
Sxnan commented on PR #24088: URL: https://github.com/apache/flink/pull/24088#issuecomment-1895529022 @1996fanrui Thanks for the update. LGTM! -- 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-34080][configuration] Simplify the Configuration [flink]
1996fanrui commented on code in PR #24088: URL: https://github.com/apache/flink/pull/24088#discussion_r1455012446 ## flink-core/src/main/java/org/apache/flink/configuration/Configuration.java: ## Review Comment: I created FLINK-34130 to follow 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-34080][configuration] Simplify the Configuration [flink]
Sxnan commented on code in PR #24088:
URL: https://github.com/apache/flink/pull/24088#discussion_r1454945796
##
flink-core/src/main/java/org/apache/flink/configuration/Configuration.java:
##
Review Comment:
That makes sense. However,
[FLINK-34082](https://issues.apache.org/jira/browse/FLINK-34082) is for
removing the deprecated methods. How about we create another ticket to ensure
that we mark them as `Internal` in 2.0?
##
flink-core/src/main/java/org/apache/flink/configuration/Configuration.java:
##
@@ -179,7 +182,9 @@ public String getString(String key, String defaultValue) {
*
* @param configOption The configuration option
* @return the (default) value associated with the given config option
+ * @deprecated use {@link #get(ConfigOption)} or {@link
#getOptional(ConfigOption)}
*/
+@Deprecated
Review Comment:
I just found out that we replace directly in #23456. If both ways exist, and
we don't have a clear guide to make the choice, I am fine with either way. Feel
free to close the comment.
--
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-34080][configuration] Simplify the Configuration [flink]
1996fanrui commented on code in PR #24088:
URL: https://github.com/apache/flink/pull/24088#discussion_r1454841243
##
flink-core/src/main/java/org/apache/flink/configuration/Configuration.java:
##
Review Comment:
I try to mark it before, but the CI will be failed after marking.
## Reason:
1. All methods are `@Public`, if method without any annotation and current
class is marked as `@Public`.
2. In the minor version, flink doesn't allow one method is changed from
`@Public` to `@Internal`.
I think it can be done in Flink-2.0 (FLINK-34082).
##
flink-core/src/test/java/org/apache/flink/configuration/ConfigurationTest.java:
##
@@ -554,6 +554,45 @@ void testMapParserErrorDoesNotLeakSensitiveData() {
.doesNotContain("secret_value"));
}
+@TestTemplate
+void testGetWithOverrideDefault() {
+final Configuration conf = new Configuration();
Review Comment:
Good catch, updated!
##
flink-core/src/main/java/org/apache/flink/configuration/Configuration.java:
##
@@ -179,7 +182,9 @@ public String getString(String key, String defaultValue) {
*
* @param configOption The configuration option
* @return the (default) value associated with the given config option
+ * @deprecated use {@link #get(ConfigOption)} or {@link
#getOptional(ConfigOption)}
*/
+@Deprecated
Review Comment:
Sorry, I don't know what difference between them?
I added `@Deprecated` due to https://github.com/apache/flink/pull/23758.
I see it added `@Deprecated` to `RestartStrategies` class directly.
--
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-34080][configuration] Simplify the Configuration [flink]
Sxnan commented on code in PR #24088:
URL: https://github.com/apache/flink/pull/24088#discussion_r1454790994
##
flink-core/src/main/java/org/apache/flink/configuration/Configuration.java:
##
Review Comment:
We should mark get/setBytes methods as `@Internal`
##
flink-core/src/test/java/org/apache/flink/configuration/ConfigurationTest.java:
##
@@ -554,6 +554,45 @@ void testMapParserErrorDoesNotLeakSensitiveData() {
.doesNotContain("secret_value"));
}
+@TestTemplate
+void testGetWithOverrideDefault() {
+final Configuration conf = new Configuration();
Review Comment:
Should we pass the `standardYaml` parameter to construct the
`Configuration`, like the other tests do?
##
flink-core/src/main/java/org/apache/flink/configuration/Configuration.java:
##
@@ -179,7 +182,9 @@ public String getString(String key, String defaultValue) {
*
* @param configOption The configuration option
* @return the (default) value associated with the given config option
+ * @deprecated use {@link #get(ConfigOption)} or {@link
#getOptional(ConfigOption)}
*/
+@Deprecated
Review Comment:
I think we can replace the `@PublicEvolving` with `@Deprecated` directly.
Same for the other methods.
--
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-34080][configuration] Simplify the Configuration [flink]
flinkbot commented on PR #24088: URL: https://github.com/apache/flink/pull/24088#issuecomment-1891392365 ## CI report: * 39fc4c7369deceabffb7bc2496300e8a8061faba 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-34080][configuration] Simplify the Configuration [flink]
1996fanrui opened a new pull request, #24088: URL: https://github.com/apache/flink/pull/24088 ## What is the purpose of the change See the part 2.2 of [FLIP-405](https://cwiki.apache.org/confluence/x/6Yr5E) ## Brief change log - [FLINK-34080][configuration] Add the `T get(ConfigOption configOption, T overrideDefault)` for Configuration - [FLINK-34080][configuration] Deprecate all getXxx and setXxx methods for Configuration - [FLINK-34080][configuration] Mark `setBytes` and `getBytes` of Configuration as `@Internal` - [FLINK-34080][configuration] Remove the `@Deprecated` for `getString(String key, String defaultValue)` of Configuration ## Verifying this change - Added `ConfigurationTest#testGetWithOverrideDefault` - Improved the `DelegatingConfigurationTest#testGetWithOverrideDefault` ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): no - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: yes - The serializers: no - The runtime per-record code paths (performance sensitive): no - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no - The S3 file system connector: no ## Documentation - Does this pull request introduce a new feature? 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
