You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "ldadima (via GitHub)" <gi...@apache.org> on 2023/09/05 06:39:11 UTC
[GitHub] [flink] ldadima opened a new pull request, #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
ldadima opened a new pull request, #23355:
URL: https://github.com/apache/flink/pull/23355
## What is the purpose of the change
To fix [FLINK-33034](https://issues.apache.org/jira/browse/FLINK-33034)
## Verifying this change
StateBackendTestBase#testGetKeysAndNamespaces
## 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)
- If yes, how is the feature documented? (not applicable)
--
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
[GitHub] [flink] ldadima commented on pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "ldadima (via GitHub)" <gi...@apache.org>.
ldadima commented on PR #23355:
URL: https://github.com/apache/flink/pull/23355#issuecomment-1709469228
> > Thanks for the pr. I have seen you also backported to 1.15. I think it's a minor fix for UT so that it's fine that we just fix it in the master.
>
> Ok. Thank you for taking on the MR review
But I think fixing this test could be added to more than just the master. Because, for example, in our case this test didn't work although it should have failed. Also I think it would be good to add to this test a check after restoring the state
--
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
[GitHub] [flink] ldadima commented on a diff in pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "ldadima (via GitHub)" <gi...@apache.org>.
ldadima commented on code in PR #23355:
URL: https://github.com/apache/flink/pull/23355#discussion_r1318044943
##########
flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendTestBase.java:
##########
@@ -500,6 +493,7 @@ public void testGetKeysAndNamespaces() throws Exception {
keysByNamespace.computeIfAbsent(entry.f1, k -> new HashSet<>());
assertTrue("Duplicate key for namespace", keys.add(entry.f0));
});
+ assertThat(keysByNamespace.size(), is(namespaces.length));
Review Comment:
This assert will be different from the others in the test. I think it's gonna look weird
--
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
[GitHub] [flink] flinkbot commented on pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #23355:
URL: https://github.com/apache/flink/pull/23355#issuecomment-1706032446
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "994d5ce25429b208f3fb5788bbcb94a65b31d44f",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "994d5ce25429b208f3fb5788bbcb94a65b31d44f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 994d5ce25429b208f3fb5788bbcb94a65b31d44f 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
[GitHub] [flink] ldadima commented on pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "ldadima (via GitHub)" <gi...@apache.org>.
ldadima commented on PR #23355:
URL: https://github.com/apache/flink/pull/23355#issuecomment-1715385100
> Thanks for the update. Could you rebase your pr with newest master ? There are some conflicts due to [FLINK-32854](https://issues.apache.org/jira/browse/FLINK-32854) which migrates this class from Junit 4 to Junit 5. So you could also update the assert into the style of Junit 5 as we disscussed above.
Done. Thanks for review
--
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
[GitHub] [flink] masteryhx closed pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "masteryhx (via GitHub)" <gi...@apache.org>.
masteryhx closed pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
URL: https://github.com/apache/flink/pull/23355
--
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
[GitHub] [flink] ldadima commented on pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "ldadima (via GitHub)" <gi...@apache.org>.
ldadima commented on PR #23355:
URL: https://github.com/apache/flink/pull/23355#issuecomment-1709680878
> > > > Спасибо за пр. Я видел, что вас также перенесли на версию 1.15. Я думаю, что это небольшое исправление для UT, так что можно просто исправить это в мастере.
> > >
> > >
> > > Хорошо. Благодарим вас за отзыв о MR.
> >
> >
> > Но я думаю, что исправление этого теста можно было бы добавить не только к мастеру. Потому что, например, в нашем случае этот тест не сработал, хотя должен был провалиться.
>
> IIUC, это всего лишь вопрос теста. И это должно влиять только на тест, а не на основной путь кода, верно?
>
> > Также думаю было бы неплохо добавить в этот тест проверку после восстановления состояния
>
> Спасибо за совет. Для меня это имеет смысл, хотели бы вы также предоставить такую проверку?
> > > > Thanks for the pr. I have seen you also backported to 1.15. I think it's a minor fix for UT so that it's fine that we just fix it in the master.
> > >
> > >
> > > Ok. Thank you for taking on the MR review
> >
> >
> > But I think fixing this test could be added to more than just the master. Because, for example, in our case this test didn't work although it should have failed.
>
> IIUC, it's just an issue of the test. And it should only affects the test not the main code path, right ?
Yes, but this error in the test may affect the correctness of the state backend improvements in the future (as for us).
>
> > Also I think it would be good to add to this test a check after restoring the state
>
> Thanks for the advice. This makes sense to me, would you like also to provide such verification?
Yes, no problem
--
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
[GitHub] [flink] ldadima commented on a diff in pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "ldadima (via GitHub)" <gi...@apache.org>.
ldadima commented on code in PR #23355:
URL: https://github.com/apache/flink/pull/23355#discussion_r1318061178
##########
flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendTestBase.java:
##########
@@ -500,6 +493,7 @@ public void testGetKeysAndNamespaces() throws Exception {
keysByNamespace.computeIfAbsent(entry.f1, k -> new HashSet<>());
assertTrue("Duplicate key for namespace", keys.add(entry.f0));
});
+ assertThat(keysByNamespace.size(), is(namespaces.length));
Review Comment:
On Flink [website](https://flink.apache.org/how-to-contribute/code-style-and-quality-common/) we can find this
"And instead use:"
```
assertThat(list)
.hasSize(10)
.allMatch(item -> item.length() < 10);
```
--
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
[GitHub] [flink] ldadima commented on pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "ldadima (via GitHub)" <gi...@apache.org>.
ldadima commented on PR #23355:
URL: https://github.com/apache/flink/pull/23355#issuecomment-1706293761
>
@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
[GitHub] [flink] ldadima commented on pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "ldadima (via GitHub)" <gi...@apache.org>.
ldadima commented on PR #23355:
URL: https://github.com/apache/flink/pull/23355#issuecomment-1706240136
@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
[GitHub] [flink] masteryhx commented on a diff in pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "masteryhx (via GitHub)" <gi...@apache.org>.
masteryhx commented on code in PR #23355:
URL: https://github.com/apache/flink/pull/23355#discussion_r1318142694
##########
flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendTestBase.java:
##########
@@ -500,6 +493,7 @@ public void testGetKeysAndNamespaces() throws Exception {
keysByNamespace.computeIfAbsent(entry.f1, k -> new HashSet<>());
assertTrue("Duplicate key for namespace", keys.add(entry.f0));
});
+ assertThat(keysByNamespace.size(), is(namespaces.length));
Review Comment:
What you mentioned is a good way for Junit 5 which tells us not to assert in the for loop for collection.
What you added in Junit 4 is not a same way, just as the website said, 'Don’t use Hamcrest'.
And it's a deprecated method.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendTestBase.java:
##########
@@ -500,6 +493,7 @@ public void testGetKeysAndNamespaces() throws Exception {
keysByNamespace.computeIfAbsent(entry.f1, k -> new HashSet<>());
assertTrue("Duplicate key for namespace", keys.add(entry.f0));
});
+ assertThat(keysByNamespace.size(), is(namespaces.length));
Review Comment:
What you mentioned is a good way for Junit 5 which tells us not to assert in the for loop for collection.
What you added in Junit 4 is not a same way, just as the website said, 'Don’t use Hamcrest'.
And it's a deprecated method.
--
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
[GitHub] [flink] ldadima commented on a diff in pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "ldadima (via GitHub)" <gi...@apache.org>.
ldadima commented on code in PR #23355:
URL: https://github.com/apache/flink/pull/23355#discussion_r1318044943
##########
flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendTestBase.java:
##########
@@ -500,6 +493,7 @@ public void testGetKeysAndNamespaces() throws Exception {
keysByNamespace.computeIfAbsent(entry.f1, k -> new HashSet<>());
assertTrue("Duplicate key for namespace", keys.add(entry.f0));
});
+ assertThat(keysByNamespace.size(), is(namespaces.length));
Review Comment:
This test will be different from the others in the test. I think it's gonna look weird
--
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
[GitHub] [flink] masteryhx commented on pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "masteryhx (via GitHub)" <gi...@apache.org>.
masteryhx commented on PR #23355:
URL: https://github.com/apache/flink/pull/23355#issuecomment-1709561911
> > > Thanks for the pr. I have seen you also backported to 1.15. I think it's a minor fix for UT so that it's fine that we just fix it in the master.
> >
> >
> > Ok. Thank you for taking on the MR review
>
> But I think fixing this test could be added to more than just the master. Because, for example, in our case this test didn't work although it should have failed.
IIUC, it's just an issue of the test.
And it should only affects the test not the main code path, right ?
> Also I think it would be good to add to this test a check after restoring the state
Thanks for the advice.
This makes sense to me, would you like also to provide such verification?
--
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
[GitHub] [flink] ldadima commented on pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "ldadima (via GitHub)" <gi...@apache.org>.
ldadima commented on PR #23355:
URL: https://github.com/apache/flink/pull/23355#issuecomment-1706295010
> ## CI report:
> * Unknown: [CANCELED](TBD)
> * [bcf37a5](https://github.com/apache/flink/commit/bcf37a5af73e2c71fbafbccbaae84b7ec686f066) UNKNOWN
>
> Bot commands
@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
[GitHub] [flink] masteryhx commented on a diff in pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "masteryhx (via GitHub)" <gi...@apache.org>.
masteryhx commented on code in PR #23355:
URL: https://github.com/apache/flink/pull/23355#discussion_r1318040628
##########
flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendTestBase.java:
##########
@@ -500,6 +493,7 @@ public void testGetKeysAndNamespaces() throws Exception {
keysByNamespace.computeIfAbsent(entry.f1, k -> new HashSet<>());
assertTrue("Duplicate key for namespace", keys.add(entry.f0));
});
+ assertThat(keysByNamespace.size(), is(namespaces.length));
Review Comment:
```suggestion
assertEquals("Unexpected size of keys and namespaces", namespaces.length, keysByNamespace.size());
```
--
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
[GitHub] [flink] ldadima commented on pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "ldadima (via GitHub)" <gi...@apache.org>.
ldadima commented on PR #23355:
URL: https://github.com/apache/flink/pull/23355#issuecomment-1709449954
> Thanks for the pr. I have seen you also backported to 1.15. I think it's a minor fix for UT so that it's fine that we just fix it in the master.
Ok. Thank you for taking on the MR review
--
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
[GitHub] [flink] ldadima commented on pull request #23355: [FLINK-33034][runtime] Correct ValueState creating for StateBackendTestBase#testGetKeysAndNamespaces
Posted by "ldadima (via GitHub)" <gi...@apache.org>.
ldadima commented on PR #23355:
URL: https://github.com/apache/flink/pull/23355#issuecomment-1709716553
>
> > Also I think it would be good to add to this test a check after restoring the state
>
> Thanks for the advice. This makes sense to me, would you like also to provide such verification?
Done
--
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