Re: [PR] [FLINK-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
1996fanrui merged PR #24004: URL: https://github.com/apache/flink/pull/24004 -- 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-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
1996fanrui merged PR #24005: URL: https://github.com/apache/flink/pull/24005 -- 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-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
1996fanrui merged PR #23994: URL: https://github.com/apache/flink/pull/23994 -- 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-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
1996fanrui commented on PR #24005: URL: https://github.com/apache/flink/pull/24005#issuecomment-1871024583 @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-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
flinkbot commented on PR #24005: URL: https://github.com/apache/flink/pull/24005#issuecomment-1870792748 ## CI report: * ab9a7e1dd0bd7b452962e1530cdc311af87f482d 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
Re: [PR] [FLINK-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
flinkbot commented on PR #24004: URL: https://github.com/apache/flink/pull/24004#issuecomment-1870792692 ## CI report: * f6d8713420217a9e2abcaf8e29a5461b5e89f00b 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-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
1996fanrui opened a new pull request, #24005: URL: https://github.com/apache/flink/pull/24005 backporting https://github.com/apache/flink/pull/23994 to 1.17 -- 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-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
1996fanrui opened a new pull request, #24004: URL: https://github.com/apache/flink/pull/24004 backporting https://github.com/apache/flink/pull/23994 to 1.18 -- 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-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
RocMarshal commented on PR #23994: URL: https://github.com/apache/flink/pull/23994#issuecomment-1869917144 > It's indeed a bug. The removeConfig and removeKey of DelegatingConfiguration missed the prefix as well. I didn't notice them in the beginning. Would you like to fix it? If yes, feel free to take it, and I'm glad to help review. Thanks you for your confirmation . my pleasure to do it( by https://issues.apache.org/jira/browse/FLINK-33947). -- 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-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
1996fanrui commented on PR #23994:
URL: https://github.com/apache/flink/pull/23994#issuecomment-1869905090
Thanks @RocMarshal for the quick review!
> if we run the followed case, will be our expected results consistent with
the current fixed results?
>
> ```
> @Test
> void testUnknownCases() {
> Configuration original = new Configuration();
> final DelegatingConfiguration delegatingConf =
> new DelegatingConfiguration(original, "prefix.");
>
> // Test for integer
> ConfigOption integerOption =
>
ConfigOptions.key("integer.key").intType().noDefaultValue();
>
> // integerOption doesn't exist in delegatingConf, and it should be
overrideDefault.
> original.setInteger(integerOption, 1);
> assertThat(delegatingConf.getInteger(integerOption,
2)).isEqualTo(2);
>
> // integerOption exists in delegatingConf, and it should be value
that set before.
> delegatingConf.setInteger(integerOption, 3);
> assertThat(delegatingConf.getInteger(integerOption,
2)).isEqualTo(3);
>
> delegatingConf.removeConfig(integerOption);
> System.out.println(delegatingConf.get(integerOption));
>
> }
> ```
>
> If not, it may be a bug ?
It's indeed a bug. The `removeConfig` and `removeKey` of
`DelegatingConfiguration` missed the `prefix` as well. I didn't notice them in
the beginning. Would you like to fix it? If yes, feel free to take it, and I'm
glad to help review.
Also, I have addressed your other comments. Please help double-check in your
free time, thanks~
--
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-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
RocMarshal commented on code in PR #23994:
URL: https://github.com/apache/flink/pull/23994#discussion_r1436698086
##
flink-core/src/test/java/org/apache/flink/configuration/DelegatingConfigurationTest.java:
##
@@ -136,15 +136,65 @@ public void
testDelegationConfigurationToMapConsistentWithAddAllToProperties() {
mapProperties.put(entry.getKey(), entry.getValue());
}
// Verification
-assertEquals(properties, mapProperties);
+assertThat(mapProperties).isEqualTo(properties);
}
@Test
-public void testSetReturnsDelegatingConfiguration() {
+void testSetReturnsDelegatingConfiguration() {
final Configuration conf = new Configuration();
final DelegatingConfiguration delegatingConf = new
DelegatingConfiguration(conf, "prefix.");
Assertions.assertThat(delegatingConf.set(CoreOptions.DEFAULT_PARALLELISM, 1))
.isSameAs(delegatingConf);
}
+
+@Test
+void testGetWithOverrideDefault() {
Review Comment:
I like the test case!
##
flink-core/src/test/java/org/apache/flink/configuration/DelegatingConfigurationTest.java:
##
@@ -115,11 +115,11 @@ public void testDelegationConfigurationWithPrefix() {
configuration = new DelegatingConfiguration(backingConf, prefix);
keySet = configuration.keySet();
-assertTrue(keySet.isEmpty());
+assertThat(keySet).isEmpty();
Review Comment:
how about
```
assertThat(configuration.keySet()).isEmpty();
```
##
flink-core/src/test/java/org/apache/flink/configuration/DelegatingConfigurationTest.java:
##
@@ -103,8 +103,8 @@ public void testDelegationConfigurationWithPrefix() {
DelegatingConfiguration configuration = new
DelegatingConfiguration(backingConf, prefix);
Set keySet = configuration.keySet();
-assertEquals(keySet.size(), 1);
-assertEquals(keySet.iterator().next(), expectedKey);
+assertThat(keySet).hasSize(1);
+assertThat(expectedKey).isEqualTo(keySet.iterator().next());
Review Comment:
how about :
```
assertThat(configuration.keySet()).containsExactly(expectedKey);
```
--
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-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
flinkbot commented on PR #23994: URL: https://github.com/apache/flink/pull/23994#issuecomment-1869458088 ## CI report: * 19de85716cc7962c47836d5d11859c333d4d454b 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-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods [flink]
1996fanrui opened a new pull request, #23994: URL: https://github.com/apache/flink/pull/23994 ## What is the purpose of the change DelegatingConfiguration misses the perfix for some methods, such as: - DelegatingConfiguration#getInteger(org.apache.flink.configuration.ConfigOption, int) - DelegatingConfiguration#getLong(org.apache.flink.configuration.ConfigOption, long) - DelegatingConfiguration#getBoolean(org.apache.flink.configuration.ConfigOption, boolean) - DelegatingConfiguration#getFloat(org.apache.flink.configuration.ConfigOption, float) - DelegatingConfiguration#getDouble(org.apache.flink.configuration.ConfigOption, double) ## Brief change log - [FLINK-33942][configuration][junit5-migration] Migrate DelegatingConfigurationTest to Junit5 and Assertj - [FLINK-33942][configuration] Fix the bug that DelegatingConfiguration misses the prefix in some get methods - [FLINK-33942][configuration][refactor] Using ConfigOption instead of string key in DelegatingConfiguration ## Verifying this change This change added tests and can be verified as follows: - *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)`: no - 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 -- 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
