Repository navigation
Conversation
JackieTien97
left a comment
There was a problem hiding this comment.
Requesting changes for two confirmed issues: enabling continuous assertions makes an existing lifecycle test fail, and the new auto-drop polling can interpret an RPC error as successful deletion. Reproduction details and suggested fixes are inline. The three new IoTDBTestUtilsIT regression tests passed.
| }) | ||
| .until(() -> true); |
There was a problem hiding this comment.
[P1] Fix the remaining lifecycle expectations when enabling continuous assertions
This terminal await also activates the previously dormant checks in IoTDBPipeLifeCycleIT.testLifeCycleWithHistoryDisabled, but that caller has not been updated. After stopPipe("p1"), root.ln0 is created only on the sender, while the receiver retains the two existing series under root.db. The receiver's series count under root.ln0, series count under root.db, and database count for root.ln0 should therefore be 0, 2, and 0; the assertions at lines 227–238 still expect 1 for all three.
I reproduced the failure from a clean reactor build with MultiClusterIT and two 1-ConfigNode/3-DataNode clusters: the first incorrect assertion fails at IoTDBPipeLifeCycleIT.java:227, with actual 0 versus expected 1. Please update all three expectations together with this helper change so the existing tree-model pipe test remains runnable.
There was a problem hiding this comment.
Applied in f15d245. The lifecycle assertions now expect 0 series under root.ln0, 2 under root.db, and 0 databases under root.ln0. Verified testLifeCycleWithHistoryDisabled with two 1-ConfigNode/3-DataNode clusters and the original observation periods; it passed. The local run used 32 MiB initial node heaps, Serial GC, and ActiveProcessorCount=2 to fit available memory; maximum node heaps and cluster topology were unchanged.
| final List<TShowPipeInfo> showPipeResult = | ||
| client.showPipe( | ||
| new TShowPipeReq() | ||
| .setIsTableModel(true) | ||
| .setUserName(SessionConfig.DEFAULT_USER)) | ||
| .pipeInfoList; |
There was a problem hiding this comment.
[P2] Check the ShowPipe response status before accepting an empty list
The direct RPC does not translate a non-success TSStatus into an exception as the JDBC query did. In PipeTaskCoordinator.showPipes(), the ConsensusException handler returns EXECUTE_STATEMENT_ERROR together with an empty pipeInfoList. Reading only the list makes both noneMatch(...) and isEmpty() accept that failed read as successful pipe deletion, even though deletion has not been established.
I verified this using the production error-response conversion: a response with status 301 passes both removal predicates. Please assert that the response status is TSStatusCode.SUCCESS_STATUS before examining the list, in all four new polling blocks across the table-model and tree-model auto-drop tests.
There was a problem hiding this comment.
Applied in f15d245 and 3b565d1. All four newly introduced auto-drop polling blocks now assert SUCCESS_STATUS before examining pipeInfoList. The same check also covers the two existing direct-RPC polls in the tree-model test, so all six polls in these two classes validate status first. All six IoTDBPipeAutoDropIT methods passed with MultiClusterIT (two 1-ConfigNode/3-DataNode clusters per test). Local runs used 32 MiB initial node heaps, Serial GC, and ActiveProcessorCount=2; maximum node heaps and cluster topology were unchanged.
|
Automated preliminary correctness review (Codex CLI, based on PR head
This review is AI-assisted and not an approval or request for changes. Please verify each scenario before acting. |
Description
Cluster ITs can report
InconsistentDataExceptionwheninformation_schema.drop_table_procedureschanges fromRUNNABLEtoSUCCESSbetween queries to different DataNodes. DROP TABLE also acknowledges submission before deletion finishes, which can race with missing-table assertions, database cleanup, and historical pipe creation.OVERLAP_WITH_EXISTING_TASKin the deletion, insert-query, CTE, explain-analyze, and pipe metadata tests. Wait for table deletion before starting the historical transfer inIoTDBPipeWithLoadIT.SHOW PIPESresult set, which could falsely indicate successful deletion. Table pipe checks explicitly select the table model.TestUtils:assertAlwaysFailand all three polling overloads ofassertDataAlwaysOnEnvpreviously configuredfailFastwithout a terminal await operation, so no queries ran. Execute checks throughout the requested observation period, select the correct SQL dialect, and compare flush timing in milliseconds. Correct the dormant callers' result headers, SQL dialects, and current privilege names; wait for permission propagation where the expected result is asynchronous. Requested observation periods, including explicit 600-second calls, now actually run.IoTDBTestUtilsITregression coverage for observation duration, rejected result mismatches, failure callbacks, and unexpected query success.The DROP TABLE call sites were reviewed against
origin/master(9a078cc105e), including JDBC, relational session, pipe, audit-log, and C++ ITs. Same-name CREATE TABLE already waits for overlapping DDL on the server; those paths do not need additional client polling. The relational session cleanup retry is already present upstream.Validation
Spotless, integration-test compilation, Checkstyle, and license checks passed.
52 distinct targeted IT methods passed with no remaining failures or errors:
TableClusterITTableSimpleITMultiClusterITThe counts exclude repeat runs of the same method. C++ call sites were reviewed statically; the full IT suite was not run.
The new helper regression tests run with
TableClusterIT(1 ConfigNode and 3 DataNodes):Local runs constrained the test-driver heap to 256-512 MiB. Final Pipe runs also used
-XX:ActiveProcessorCount=2, Serial GC, and a 32 MiB initial node heap to fit the machine's memory limits; DataNode maximum heap and direct-memory limits remained at 256 MiB. Cluster topologies and assertion durations were preserved.