Repository navigation
Subscription: add topic owner epoch fencing - #17780
Merged
Merged
Conversation
Collaborator
|
Findings
( 和 TopicMetaKeeper.java#L112 (https://github.com/apache/iotdb/blob/76f2a882411ba790f7db290740f0232a1e5c4022/iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/subscription/meta/topic/TopicMetaKeeper.java#L112)。建议改成版本化或长度前缀格式，或只从
( 和 SubscriptionInfo.java#L340 (https://github.com/apache/iotdb/blob/76f2a882411ba790f7db290740f0232a1e5c4022/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/subscription/SubscriptionInfo.java#L340)。建议在
(https://github.com/apache/iotdb/blob/76f2a882411ba790f7db290740f0232a1e5c4022/iotdb-client/subscription/src/main/java/org/apache/iotdb/session/subscription/consumer/base/AbstractSubscriptionProvider.java#L445)。建议显式处理 |
VGalaxies
force-pushed
the
subscription-topic-owner-fencing
branch
from
May 28, 2026 10:35
76f2a88 to
b6086b2
Compare
Contributor
Author
|
Thanks @Caideyipi, addressed the three findings in b6086b2.
Local verification passed:
The Sonar duplication check is queued again on the new commit; I will follow up if the rerun still fails. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #17780 +/- ##
============================================
+ Coverage 40.68% 40.99% +0.30%
+ Complexity 2620 318 -2302
============================================
Files 5244 5253 +9
Lines 362374 364620 +2246
Branches 46653 47110 +457
============================================
+ Hits 147419 149460 +2041
- Misses 214955 215160 +205 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jt2594838
reviewed
Jun 2, 2026
jt2594838
left a comment
jt2594838
left a comment
Contributor
There was a problem hiding this comment.
Add an IT to show a complete use case.
VGalaxies
force-pushed
the
subscription-topic-owner-fencing
branch
from
June 2, 2026 09:46
b6086b2 to
25c34c2
Compare
Contributor
Author
|
Addressed the latest review comments in 25c34c2.
Local verification:
|
jt2594838
reviewed
Jun 3, 2026
jt2594838
reviewed
Jun 8, 2026
jt2594838
left a comment
jt2594838
left a comment
Contributor
There was a problem hiding this comment.
Have you waited for all leases to expire before transferring the ownership?
Contributor
Author
|
Addressed the lease-transfer review comment in 82049d7. Topic ownership transfer now checks the current topic owner lease before replacing owner metadata: if the current owner has an unexpired Also addressed the inline comments in the same commit:
Local verification passed:
|
jt2594838
approved these changes
Jun 9, 2026
jt2594838
reviewed
Jun 11, 2026
Land the technical-review corrections together: - Seek fencing in SubscriptionReceiverV1 before any position-mutating op. - Move the owner lease off the node heartbeat onto a dedicated, fine-grained SubscriptionOwnerLeaseSyncer pushing relative remaining durations. - Make the absolute lease expiry DataNode-local runtime state only: it is set solely from the lease heartbeat, never an attribute, never serialized, and not part of equals/hashCode. CN persists only owner-id, owner-epoch, max-owner-epoch and owner-lease-duration-ms. - Add globally-monotonic, never-reused owner epochs via max-owner-epoch that survives clearOwner. - Drain owner transfers via a ConfigNode admission-wait (blockSince + L + H) instead of CN-side lease-not-expired rejection/retry. - Remove the dead absolute-timestamp lease machinery (renewOwnerLease*, getOwnerLeaseRemainingTimeMs, isActiveOwnerLease/isOwnerTransfer, the owner-lease-expire-time-ms attribute key, the alterTopic retry loop) and update the unit tests accordingly.
Address the i18n review comments: the owner-validation exception messages in TopicMeta now use SubscriptionMessages (en/zh), and the alter-topic warn in SubscriptionCoordinator uses ManagerMessages. The previously flagged renewTopicOwnerLease message no longer exists after the lease rework.
AbstractSubscriptionLocalIT already enables subscription, so the end-to-end stale-owner-fencing + current-owner-consume scenario runs; verified locally via mvn verify -Dit.test=IoTDBSubscriptionTopicOwnerIT (tests=2, failures=0, errors=0, skipped=0).
Drop 'SN' wording from the PR's new code: owner-id values sn1/sn2 -> owner1/owner2, consumer ids and vars stale_sn/current_sn/oldSnConsumer/newSnConsumer -> owner-based names, and the SN/DN/CN comment -> owner/DataNode/ConfigNode. Pure rename; all affected unit tests still pass.
jt2594838
approved these changes
Jun 12, 2026
Reject owner-lease-duration-ms below a configurable floor at topic create/alter admission, so the lease stays well above the ~5s renewal heartbeat and a missed beat cannot falsely fence a healthy owner. - New config subscription_owner_lease_duration_ms_min (CommonConfig, default 60000), loaded by CommonDescriptor, exposed via SubscriptionConfig. - Enforced in SubscriptionInfo.validateTopicConfig (create + alter admission), with an en/zh ManagerMessages constant. Deserialize stays lenient (>0) so already-persisted topics still load if the floor is later raised. - IT framework: setSubscriptionOwnerLeaseDurationMsMin on the CommonConfig interface + Mpp/MppShared/Remote impls; the two owner ITs lower the floor in setUp to keep their short leases (and stay fast). Verified the local IT passes (tests=2, failures=0) end-to-end.
Address review feedback: the ConfigNode admission wait is (lease duration + one renewal/heartbeat interval) measured on its own clock, not the lease duration alone, since a renewal may have been in flight to a DataNode just before renewal was stopped. Updated the drain test comment to reflect this; the >= lease-duration lower bound still holds because the actual wait is duration + one interval.
|
jt2594838
approved these changes
Jun 12, 2026
MileaRobertStefan
pushed a commit
to MileaRobertStefan/iotdb
that referenced
this pull request
Jun 26, 2026
* Subscription: add topic owner epoch fencing * Implement alter topic owner entrypoints * Add topic owner update show test * Subscription: address topic owner review comments * Implement subscription topic owner lease renewal * Use duration-based subscription owner lease renewal * Renew subscription owner leases in ConfigNode heartbeat * Rework subscription topic owner epoch fencing and lease Land the technical-review corrections together: - Seek fencing in SubscriptionReceiverV1 before any position-mutating op. - Move the owner lease off the node heartbeat onto a dedicated, fine-grained SubscriptionOwnerLeaseSyncer pushing relative remaining durations. - Make the absolute lease expiry DataNode-local runtime state only: it is set solely from the lease heartbeat, never an attribute, never serialized, and not part of equals/hashCode. CN persists only owner-id, owner-epoch, max-owner-epoch and owner-lease-duration-ms. - Add globally-monotonic, never-reused owner epochs via max-owner-epoch that survives clearOwner. - Drain owner transfers via a ConfigNode admission-wait (blockSince + L + H) instead of CN-side lease-not-expired rejection/retry. - Remove the dead absolute-timestamp lease machinery (renewOwnerLease*, getOwnerLeaseRemainingTimeMs, isActiveOwnerLease/isOwnerTransfer, the owner-lease-expire-time-ms attribute key, the alterTopic retry loop) and update the unit tests accordingly. * Move subscription owner fencing messages into i18n constants Address the i18n review comments: the owner-validation exception messages in TopicMeta now use SubscriptionMessages (en/zh), and the alter-topic warn in SubscriptionCoordinator uses ManagerMessages. The previously flagged renewTopicOwnerLease message no longer exists after the lease rework. * Reword owner-lease syncer TODO to satisfy the todo-check CI gate * Enable the local topic-owner fencing IT (drop stale @ignore) AbstractSubscriptionLocalIT already enables subscription, so the end-to-end stale-owner-fencing + current-owner-consume scenario runs; verified locally via mvn verify -Dit.test=IoTDBSubscriptionTopicOwnerIT (tests=2, failures=0, errors=0, skipped=0). * Rename SN owner identifiers to neutral owner names in tests Drop 'SN' wording from the PR's new code: owner-id values sn1/sn2 -> owner1/owner2, consumer ids and vars stale_sn/current_sn/oldSnConsumer/newSnConsumer -> owner-based names, and the SN/DN/CN comment -> owner/DataNode/ConfigNode. Pure rename; all affected unit tests still pass. * Add configurable minimum owner-lease-duration floor (default 1 min) Reject owner-lease-duration-ms below a configurable floor at topic create/alter admission, so the lease stays well above the ~5s renewal heartbeat and a missed beat cannot falsely fence a healthy owner. - New config subscription_owner_lease_duration_ms_min (CommonConfig, default 60000), loaded by CommonDescriptor, exposed via SubscriptionConfig. - Enforced in SubscriptionInfo.validateTopicConfig (create + alter admission), with an en/zh ManagerMessages constant. Deserialize stays lenient (>0) so already-persisted topics still load if the floor is later raised. - IT framework: setSubscriptionOwnerLeaseDurationMsMin on the CommonConfig interface + Mpp/MppShared/Remote impls; the two owner ITs lower the floor in setUp to keep their short leases (and stay fast). Verified the local IT passes (tests=2, failures=0) end-to-end. * Clarify owner-lease drain wait depends on the heartbeat interval too Address review feedback: the ConfigNode admission wait is (lease duration + one renewal/heartbeat interval) measured on its own clock, not the lease duration alone, since a renewal may have been in flight to a DataNode just before renewal was stopped. Updated the drain test comment to reflect this; the >= lease-duration lower bound still holds because the actual wait is duration + one interval. --------- Co-authored-by: OpenAI Codex <codex@openai.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add owner-epoch fencing for subscription topics so that, after an owner
failover/transfer, stale owners can no longer mutate a topic's consumption
progress.
owner-idandowner-epoch. Consumers propagate theirowner-id/owner-epoch, and theDataNode fences stale owners on heartbeat, subscribe, poll, commit, and
seek(before any position-mutating operation).max-owner-epochattribute records the highest epoch ever granted and survives
clearOwner,so an owner change can never reuse an epoch
<=any epoch previouslygranted, even across a clear -> re-enable cycle.
over a dedicated, fine-grained
SubscriptionOwnerLeaseSyncer(independent ofthe node heartbeat) carrying a relative remaining duration. The absolute
expiry is derived and held DataNode-local at runtime only -- it is never a
topic attribute, never serialized, and not part of
equals/hashCode. Thisavoids comparing absolute timestamps across ConfigNode/DataNode clocks. The
ConfigNode persists only
owner-id,owner-epoch,max-owner-epoch, andowner-lease-duration-ms.ConfigNode stops renewing the previous owner's lease and waits
blockSince + lease-duration + heartbeat-intervalon its own clock beforeinstalling the new owner. Epoch fencing on reachable DataNodes guarantees
correctness during the wait.
Notable design points
primary fence: if no lease has been established yet (e.g. right after a
meta push), owner-id/epoch matching still fences correctly.
Tests
Unit tests (pass):
TopicDeSerTestcovers serialization, owner transfer, cross-clear epoch-reuserejection, and the DataNode-local-only lease expiry behavior;
SubscriptionInfoTestcovers epoch-rollback rejection, owner transfer viaupdated attributes, the read-only relative-duration lease collector, and
SHOW TOPICSoutput.Compilation / build validation:
Integration tests added but not run under MiniCluster in this change:
IoTDBSubscriptionTopicOwnerIT(relative-lease drain) andIoTDBSubscriptionTopicOwnerPartitionIT(multi-DataNode fencing, ConfigNodefailover epoch persistence, DataNode restart).