wuchong commented on code in PR #2370:
URL: https://github.com/apache/fluss/pull/2370#discussion_r2707393111
##########
fluss-server/src/test/java/org/apache/fluss/server/kv/KvTabletTest.java:
##########
@@ -1333,6 +1321,28 @@ private MemoryLogRecords logRecords(
DEFAULT_COMPRESSION);
}
+ private MemoryLogRecords logRecords(
Review Comment:
not used, remove
##########
fluss-client/src/test/java/org/apache/fluss/client/table/FlussTableITCase.java:
##########
@@ -1160,6 +1164,26 @@ void testInvalidColumnProjection() throws Exception {
@ParameterizedTest
@ValueSource(booleans = {true, false})
void testFirstRowMergeEngine(boolean doProjection) throws Exception {
+ Configuration conf = initConfig();
+ // To better mock the issue
https://github.com/apache/fluss/issues/2369:
+ // 1. disable remote log task so that won't read remote log.
+ // 2. Set LOG_SEGMENT_FILE_SIZE to make sure one segment before last
segment is contain
+ // empty batch at the end.
+ // In this way, if skip empty batch, the read will in stuck forever.
+ conf.set(ConfigOptions.REMOTE_LOG_TASK_INTERVAL_DURATION,
Duration.ZERO);
+ conf.set(
+ ConfigOptions.LOG_SEGMENT_FILE_SIZE,
+ new MemorySize(5 * V0_RECORD_BATCH_HEADER_SIZE));
+ conf.set(
+ ConfigOptions.CLIENT_SCANNER_LOG_FETCH_MAX_BYTES_FOR_BUCKET,
+ new MemorySize(5 * V0_RECORD_BATCH_HEADER_SIZE));
+ final FlussClusterExtension flussClusterExtension =
+ FlussClusterExtension.builder()
+ .setNumOfTabletServers(3)
+ .setClusterConf(conf)
+ .build();
+ flussClusterExtension.start();
Review Comment:
In my local environment, the original test takes about 15 seconds, but it
increases to 55 seconds after applying this PR. I think we should optimize it.
Here are several optimization opportunities:
1. The test starts a new Fluss cluster in addition to the existing one from
the test base class, which adds significant overhead.
2. The issue only occurs when projection pushdown is enabled (`doProjection
= true`), so there’s no need to test `doProjection = false`.
3. The table bucket count can be reduced to 1, as it’s sufficient for
reproducing the problem.
4. The number of empty record batches can be reduced from 10 to 2, this
should still reliably reproduce the issue while avoiding 10 unnecessary RPC
round-trips that dominate the runtime.
I suggest introducing a dedicated test class like `CustomFlussClusterITCase`
(without extending `ClientToServerITCaseBase`) for tests that require manual
cluster management, and adding a focused test method such as
`testProjectionPushdownWithEmptyBatches` that incorporates all these
optimizations.
--
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]
