zhangjun0x01 commented on a change in pull request #1936:
URL: https://github.com/apache/iceberg/pull/1936#discussion_r561491288
##########
File path:
flink/src/test/java/org/apache/iceberg/flink/TestFlinkTableSource.java
##########
@@ -685,4 +782,60 @@ public void testSqlParseError() {
AssertHelpers.assertThrows("The NaN is not supported by flink now. ",
NumberFormatException.class, () -> sql(sqlParseErrorLTE));
}
+
+ /**
+ * The sql can be executed in both streaming and batch mode, in order to get
the parallelism, we convert the flink
+ * Table to flink DataStream, so we only use streaming mode here.
+ *
+ * @throws TableNotExistException table not exist exception
+ */
+ @Test
+ public void testInferedParallelism() throws TableNotExistException {
+ Assume.assumeTrue("The execute mode should be streaming mode",
isStreamingJob);
Review comment:
> In my mind, Providing unit tests to check whether the
`inferParallelism()` is returning the expected parallelism value is enough for
this changes. Seems like The ITCase is validating the behavior of
DataStreamSource#setParallelism , we could think it's always correct because
it's a basic API in flink.
I think it’s better not to use the `inferParallelism` method to get the
parallelism to do assertion, because the `inferParallelism` method is private
and is an internal method of iceberg. Just as you
[commented](https://github.com/apache/iceberg/pull/1936#discussion_r554983424)
that it is best not to use the internal code of flink, I think we should try to
use public APIs to get information.
The current `TestFlinkTableSource` class uses batch mode for unit test. In
order not to modify too much code, we can move the `testInferedParallelism`
method to other test classes, such as `TestFlinkScan.java`.
So I think we can use `DataStream.getTransformation().getParallelism();` to
get the parallelism of the flink operator. This method is public api of flink.
Even if flink is upgraded in the future, it should not be modified. What do you
think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
