[server] Support altering table.log.ttl table option#3233
[server] Support altering table.log.ttl table option#3233Kaixuan-Duan wants to merge 1 commit intoapache:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Enables ALTER TABLE ... SET ('table.log.ttl' = ...) by allowing the option through config validation and propagating updated TTL values into the server-side remote-log expiration logic without requiring tablet recreation.
Changes:
- Allow
table.log.ttlto be altered by adding it toFlussConfigUtils.ALTERABLE_TABLE_OPTIONS. - Make
RemoteLogTabletTTL mutable/visible across threads and add update/access methods; wire TTL updates throughReplicaandReplicaManager. - Add/extend tests to validate TTL alteration persistence and runtime effect on remote-log expiration.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| fluss-common/src/main/java/org/apache/fluss/config/FlussConfigUtils.java | Allows table.log.ttl to pass ALTER-table option validation. |
| fluss-server/src/main/java/org/apache/fluss/server/log/remote/RemoteLogTablet.java | Makes TTL mutable (volatile) and adds TTL getter/updater for runtime expiration decisions. |
| fluss-server/src/main/java/org/apache/fluss/server/replica/Replica.java | Adds updateLogTtlMs to apply TTL changes to the associated RemoteLogTablet. |
| fluss-server/src/main/java/org/apache/fluss/server/replica/ReplicaManager.java | Propagates TTL values from metadata updates to online replicas. |
| fluss-server/src/test/java/org/apache/fluss/server/log/remote/RemoteLogTabletTest.java | Adds unit coverage for TTL update behavior affecting expiration results. |
| fluss-server/src/test/java/org/apache/fluss/server/log/remote/RemoteLogManagerTest.java | Adds integration-style test ensuring replica-driven TTL updates reach RemoteLogTablet. |
| fluss-client/src/test/java/org/apache/fluss/client/admin/FlussAdminITCase.java | Adds IT coverage verifying ALTER persists TTL into TableInfo and reset reverts to default. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /** Returns the current ttl in milliseconds for remote log segments. */ | ||
| @VisibleForTesting |
There was a problem hiding this comment.
getTtlMs() is annotated @VisibleForTesting but is now used by production code (Replica.updateLogTtlMs). This annotation is misleading and may encourage future refactors to remove/break the method; consider either removing @VisibleForTesting (and documenting it as part of the runtime API) or exposing a non-test accessor via RemoteLogManager/package-private API that Replica can use.
| @VisibleForTesting |
| final RemoteLogTablet remoteLogTablet; | ||
| try { | ||
| remoteLogTablet = remoteLogManager.remoteLogTablet(tableBucket); | ||
| } catch (IllegalStateException e) { | ||
| // RemoteLogTablet may not be registered yet during the early stage of replica | ||
| // creation. In that case, the alter is a no-op here because the Replica will read the | ||
| // up-to-date ttl from the persisted TableInfo when constructing the RemoteLogTablet. | ||
| LOG.warn( | ||
| "RemoteLogTablet for {} is not registered yet, skip applying new logTtlMs={}.", | ||
| tableBucket, | ||
| newTtlMs); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This calls remoteLogManager.remoteLogTablet(tableBucket), which is currently a @VisibleForTesting method and throws IllegalStateException when absent. For production code paths, it would be cleaner to add a non-test API on RemoteLogManager (e.g., returning @Nullable/Optional) or a dedicated updateLogTtlMs(tableBucket, ttl) method, instead of relying on an exception-driven control flow.
| final RemoteLogTablet remoteLogTablet; | |
| try { | |
| remoteLogTablet = remoteLogManager.remoteLogTablet(tableBucket); | |
| } catch (IllegalStateException e) { | |
| // RemoteLogTablet may not be registered yet during the early stage of replica | |
| // creation. In that case, the alter is a no-op here because the Replica will read the | |
| // up-to-date ttl from the persisted TableInfo when constructing the RemoteLogTablet. | |
| LOG.warn( | |
| "RemoteLogTablet for {} is not registered yet, skip applying new logTtlMs={}.", | |
| tableBucket, | |
| newTtlMs); | |
| return; | |
| } | |
| final RemoteLogTablet remoteLogTablet = remoteLogManager.remoteLogTablet(tableBucket); |
| // RemoteLogTablet may not be registered yet during the early stage of replica | ||
| // creation. In that case, the alter is a no-op here because the Replica will read the | ||
| // up-to-date ttl from the persisted TableInfo when constructing the RemoteLogTablet. | ||
| LOG.warn( | ||
| "RemoteLogTablet for {} is not registered yet, skip applying new logTtlMs={}.", |
There was a problem hiding this comment.
Logging this as WARN on every TTL update can be noisy in normal scenarios (e.g., remote-log tiering disabled globally or replica still initializing), and the message implies “not registered yet” even when remote logging is disabled. Consider downgrading to DEBUG (or gating on remote-log enabled) and/or refining the message so operators can distinguish disabled vs. not-yet-registered cases.
| // RemoteLogTablet may not be registered yet during the early stage of replica | |
| // creation. In that case, the alter is a no-op here because the Replica will read the | |
| // up-to-date ttl from the persisted TableInfo when constructing the RemoteLogTablet. | |
| LOG.warn( | |
| "RemoteLogTablet for {} is not registered yet, skip applying new logTtlMs={}.", | |
| // RemoteLogTablet may be unavailable when remote logging is disabled or during the | |
| // early stage of replica creation. In those cases, the alter is a no-op here because | |
| // the Replica will read the up-to-date ttl from the persisted TableInfo when | |
| // constructing the RemoteLogTablet. | |
| LOG.debug( | |
| "RemoteLogTablet for {} is unavailable; skip applying new logTtlMs={} " | |
| + "(remote logging may be disabled or the replica is still initializing).", |
| // Verify initial ttl is 7 days | ||
| long defaultTtlMs = java.time.Duration.ofDays(7).toMillis(); |
There was a problem hiding this comment.
This test hard-codes the default TTL as 7 days. Since the default is already defined in ConfigOptions.TABLE_LOG_TTL, it would be more robust to derive the expected value from the config option (as done in RemoteLogTabletTest) to avoid failures if the default ever changes.
| // Verify initial ttl is 7 days | |
| long defaultTtlMs = java.time.Duration.ofDays(7).toMillis(); | |
| // Verify initial ttl matches the configured default table.log.ttl | |
| long defaultTtlMs = ConfigOptions.TABLE_LOG_TTL.defaultValue().toMillis(); |
Purpose
Linked issue: close #3231
Brief change log
ConfigOptions.TABLE_LOG_TTL.key()toFlussConfigUtils.ALTERABLE_TABLE_OPTIONSso the validator no longer rejects ALTER on this option.RemoteLogTablet#ttlMsfromfinaltovolatileand addgetTtlMs()/updateTtlMs(long)so the remote-log expiration check can observe the new value without recreating the tablet.Replica#updateLogTtlMs(long)which looks up the correspondingRemoteLogTabletviaRemoteLogManagerand applies the new TTL, guarded by anIllegalStateExceptionfallback for the case where the tablet has not been registered yet.ReplicaManager#updateReplicaTableConfigto collect atableIdToLogTtlMsmap from the metadata diff and dispatchupdateLogTtlMsto each affected replica, mirroring howtable.log.tiered.local-segmentsis already handled.Tests
./mvnw -pl fluss-server test -Dtest='RemoteLogTabletTest,RemoteLogManagerTest,RemoteLogTTLTest'
./mvnw -pl fluss-client test -Dtest='FlussAdminITCase'
API and Format
Documentation