swuferhong commented on code in PR #3222:
URL: https://github.com/apache/fluss/pull/3222#discussion_r3557551287
##########
fluss-server/src/main/java/org/apache/fluss/server/log/LogTablet.java:
##########
@@ -554,11 +597,15 @@ public void updateRemoteLogSize(long remoteLogSize) {
}
public void updateRemoteLogEndOffset(long remoteLogEndOffset) {
+ updateRemoteLogEndOffset(remoteLogEndOffset, clock.milliseconds());
+ }
+
+ public void updateRemoteLogEndOffset(long remoteLogEndOffset, long
currentTimeMs) {
if (remoteLogEndOffset > this.remoteLogEndOffset) {
this.remoteLogEndOffset = remoteLogEndOffset;
// try to delete these segments already exist in remote storage.
- deleteSegmentsAlreadyExistsInRemote();
+ deleteSegmentsAlreadyExistsInRemote(currentTimeMs);
Review Comment:
Local cleanup is only invoked when `remoteLogEndOffset` increases. During a
remote-TTL-only update, however, the manifest end offset normally stays
unchanged, decreases, or becomes -1, so this branch is skipped.
Consequently, inactive local segments that were protected by
`table.log.tiered.local-segments` when uploaded can remain locally readable
indefinitely after their TTL has expired.
The current test uploads another segment after advancing the clock, which
increases the remote end offset and masks this path.
Could cleanup be evaluated on every retention pass, independently of whether
the remote end offset advanced? Please also add a test with no newly uploaded
segment and assert the local segment count/start offset after TTL expiration.
##########
fluss-server/src/main/java/org/apache/fluss/server/log/remote/LogTieringTask.java:
##########
@@ -127,14 +127,15 @@ private void runOnce() throws InterruptedException {
TableMetricGroup metricGroup = replica.tableMetrics();
maybeUpdateCopiedOffset(logTablet);
+ logTablet.rollActiveSegmentIfExpired(currentTimeMs);
Review Comment:
I think this is a very dangerous operation. `rollActiveSegmentIfExpired` is
only called by the leader’s `LogTieringTask`, so the new segment boundary is
not propagated to followers. For example, the leader may roll and upload `[A,
B)`, while a follower continues to keep `[A, C)` as a single segment. If that
follower later becomes the leader, `LogSegments.values()` may select and upload
the whole [A, C) segment, overlapping the existing remote segment.
`RemoteLogTablet` does not safely support this overlap: both segments remain
in the ID/manifest maps, while the start-offset map retains only one UUID. When
`[A, B)` expires, removing its start-offset entry may also remove the index for
`[A, C)`, and firstKey() may subsequently fail.
Could we either keep segment boundaries consistent across replicas or make
the remote metadata overlap/leader-epoch aware?
--
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]
