XComp commented on code in PR #23770:
URL: https://github.com/apache/flink/pull/23770#discussion_r1401771193
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/testutils/DriverTestBase.java:
##########
@@ -190,18 +190,18 @@ protected void setNumFileHandlesForSort(int
numFileHandles) {
this.numFileHandles = numFileHandles;
}
- @SuppressWarnings("rawtypes")
+ @SuppressWarnings({"rawtypes", "unchecked"})
protected void testDriver(Driver driver, Class stubClass) throws Exception
{
Review Comment:
```suggestion
protected void testDriver(Driver driver, Class<? extends S> stubClass)
throws Exception {
```
I guess, the trick to avoid adding another `\@SuppressWarnings` is to have
the generic class being specified in the method signature. :thinking: WDYT?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/testutils/DriverTestBase.java:
##########
@@ -190,18 +190,18 @@ protected void setNumFileHandlesForSort(int
numFileHandles) {
this.numFileHandles = numFileHandles;
}
- @SuppressWarnings("rawtypes")
+ @SuppressWarnings({"rawtypes", "unchecked"})
protected void testDriver(Driver driver, Class stubClass) throws Exception
{
- testDriverInternal(driver, stubClass);
+ testDriver(driver, (S) stubClass.newInstance());
Review Comment:
```suggestion
testDriver(driver, (S)
stubClass.getDeclaredConstructor().newInstance());
```
That's what the IDE is suggesting because `Class'#newInstance()` is
deprecated.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/testutils/DriverTestBase.java:
##########
@@ -190,18 +190,18 @@ protected void setNumFileHandlesForSort(int
numFileHandles) {
this.numFileHandles = numFileHandles;
}
- @SuppressWarnings("rawtypes")
+ @SuppressWarnings({"rawtypes", "unchecked"})
protected void testDriver(Driver driver, Class stubClass) throws Exception
{
Review Comment:
```suggestion
/**
* @deprecated Use {@link #testDriver(Driver, Function)} instead.
*/
@Deprecated
```
I'm wondering whether we should make this method deprecated and suggest
using your newly added method. Passing the class doesn't bring any benefits
because we call the default constructor internally anyway. :shrug:
The only downside of that one would be that all the old test classes are
using this method and would have a deprecation warning being add :roll_eyes:
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/CoGroupTaskTest.java:
##########
@@ -406,46 +328,28 @@ void testCancelCoGroupTaskWhileCoGrouping() {
getTaskConfig().setDriverPairComparator(RecordPairComparatorFactory.get());
getTaskConfig().setDriverStrategy(DriverStrategy.CO_GROUP);
- final CoGroupDriver<Record, Record, Record> testTask =
- new CoGroupDriver<Record, Record, Record>();
+ final CoGroupDriver<Record, Record, Record> testTask = new
CoGroupDriver<>();
- try {
- addInput(new UniformRecordGenerator(keyCnt, valCnt, true));
- addInput(new UniformRecordGenerator(keyCnt, valCnt, true));
- } catch (Exception e) {
- e.printStackTrace();
- fail("The test caused an exception.");
- }
+ addInput(new UniformRecordGenerator(keyCnt, valCnt, true));
+ addInput(new UniformRecordGenerator(keyCnt, valCnt, true));
- final AtomicBoolean success = new AtomicBoolean(false);
+ final OneShotLatch closeLatch = new OneShotLatch();
- Thread taskRunner =
- new Thread() {
+ CheckedThread taskRunner =
+ new CheckedThread() {
@Override
- public void run() {
- try {
- testDriver(testTask,
MockDelayingCoGroupStub.class);
- success.set(true);
- } catch (Exception ie) {
- ie.printStackTrace();
- }
+ public void go() throws Exception {
+ testDriver(testTask, new
MockDelayingCoGroupStub(closeLatch));
Review Comment:
I'm not sure whether we're keeping the same test scenario here. The old
implementation called `cancel` while `coGroup` was called. The new test
implementation is more likely having the cancel call being performed while
closing the `RichCoGroupFunction`. WDYT?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
