Repository navigation
Conversation
Treat MinimumProgressIndex as unknown for Ratis historical extraction so successful writes remain eligible for replay after a leader change. Preserve valid-index and other consensus-protocol coverage filtering. Add a regression test for initial and resumed checkpoints and the unaffected filtering behavior.
|
Initial independent self-review completed at head 1722b53, against master base a927dd6. I reviewed the complete two-file diff and the surrounding historical extraction, progress filtering, Pipe checkpoint commit ordering, TsFile progress persistence, and Ratis recovery/snapshot paths. No actionable source-behavior or test-logic issues were found, so no fixes were needed. The dedicated clean DataNode dependency-closure build and Spotless validation passed, and PipeHistoricalDataRegionTsFileAndDeletionSourceTest passed all 21 tests with 0 failures, errors, or skips, including the added Ratis regression. The added regression was not run in this local review session. GitHub CI is reported separately: some checks are still in progress, and SonarCloud Code Analysis failed on findings outside the files changed by this PR. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19053 +/- ##
============================================
+ Coverage 48.05% 48.16% +0.11%
- Complexity 604 614 +10
============================================
Files 5572 5572
Lines 410194 410336 +142
Branches 54112 54161 +49
============================================
+ Hits 197110 197637 +527
+ Misses 213084 212699 -385 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Caideyipi
changed the title
|
This update changes the PR from replaying Ratis historical TsFiles to disabling Pipe and Subscription under Ratis.
This task-specific behavior update requires a fresh independent review. The PR remains Draft at head e2d2137. |
|
Independent re-review of
Ratis itself uses Raft log replication and has no consensus-internal Pipes; internal Pipe compatibility was checked with IoTConsensusV2. The regression coverage also checks permitted metadata/deletion Pipes, rejected file-transfer side effects, and absence of a restored subscription broker. No independent assertion or test-logic defect was identified in the original PR tests. Validation:
Reviewed SHA: |
|
Independent re-review completed in one round. Reviewed SHA: Found and fixed two behavioral issues:
Validation: Spotless apply and diff checks passed. The 29-module source dependency closure was rebuilt from clean source; after correcting the new test fixture's Mockito compatibility and model dialect setup, ConfigNode was cleaned and rebuilt and the final 29-module reactor passed. All 108 tests in nine suites passed, with zero failures, errors or skips. All newly added tests were run locally. The remote attempt failed resolving a Maven extension before compilation; validation completed locally with JDK 17. This was focused unit-test/dependency-closure validation. Chinese-locale compilation was not repeated this round; no localization constants or cross-module APIs changed. Final local SHA: |
|
Independent re-review completed (one round).
|
Caideyipi
changed the title
| if (!staticMeta.getPipeName().startsWith(PipeStaticMeta.CONSENSUS_PIPE_PREFIX) | ||
| && !PipeConfigNodeAgent.isPipeDataInsertionEnabled(staticMeta.getSourceParameters())) { |
There was a problem hiding this comment.
The condition is a conflict because we cannot use IoTV2 and Ratis at the same time.
There was a problem hiding this comment.
Agreed. IoTConsensusV2 and Ratis are mutually exclusive, so the consensus-Pipe-prefix exemption is redundant. It was removed in 7936b04, along with the matching exemptions at the other admission and recovery checks. The merged branch retains that change.
| switch (requestType) { | ||
| case TRANSFER_TABLET_INSERT_NODE: | ||
| case TRANSFER_TABLET_INSERT_NODE_V2: | ||
| case TRANSFER_TABLET_RAW: | ||
| case TRANSFER_TABLET_RAW_V2: | ||
| case TRANSFER_TABLET_BINARY: | ||
| case TRANSFER_TABLET_BINARY_V2: | ||
| case TRANSFER_TABLET_BATCH: | ||
| case TRANSFER_TABLET_BATCH_V2: | ||
| case TRANSFER_TS_FILE_PIECE: | ||
| case TRANSFER_TS_FILE_SEAL: | ||
| case TRANSFER_TS_FILE_PIECE_WITH_MOD: | ||
| case TRANSFER_TS_FILE_SEAL_WITH_MOD: | ||
| return true; | ||
| default: | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Use enhanced switch without a default to avoid misusing the default value for future types.
There was a problem hiding this comment.
Applied in 35f1f0cd863. isDataInsertionRequest now uses an exhaustive switch expression with no default. All 25 current enum values are classified explicitly, and future enum additions will require an explicit classification at compile time.
| SubscriptionConfig.getInstance().getSubscriptionEnabled() | ||
| && PipeDataNodeAgent.isSubscriptionFeatureEnabled(), |
There was a problem hiding this comment.
Applied in 35f1f0cd863. The constructor now uses one isSubscriptionEnabled() helper for both timeout-checker scheduling and the dynamic subscription-enabled supplier, combining the configuration flag and the Ratis feature check.
|
Review feedback update, starting from
Validation completed locally with JDK 17:
Final local commit and confirmed source-branch/PR head: Remote CI is reported separately from the passing focused local validation:
|
|
When the DataRegion consensus protocol is Ratis, disable Subscription and only those Pipe synchronizations that capture data insertion (
data.insertin the inclusion options, after applying exclusions).data.delete-only Pipes available under Ratis, including their startup, recovery and transfer paths. Pipes whose effective options exclude data insertion remain available.Validation:
7936b04cea7): Spotless passed for DataNode and ConfigNode, and whitespace checks passed. The focused unit-test rerun was blocked before compilation while resolving the missing Maven extensioncom.gradle:develocity-maven-extension:2.6.0; no tests were executed in this rerun.