Skip to content

Conversation

iamaleksey
Copy link
Member

patch by Aleksey Yeschenko; reviewed by Alex Petrov for CASSANDRA-20710

patch by Aleksey Yeschenko; reviewed by Alex Petrov for CASSANDRA-20710
@iamaleksey iamaleksey requested a review from ifesdjeen October 3, 2025 15:22
@iamaleksey iamaleksey self-assigned this Oct 3, 2025
Copy link
Contributor

@ifesdjeen ifesdjeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

I would personally add an integration test, similar to JournalGCTest: you populate keyspace, flush all tables, run discard, and make sure everything's discarded.

into.add(segment.asStatic());
}

void selectStatic(Predicate<StaticSegment<K, V>> filter, Collection<StaticSegment<K, V>> into)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we cache static segments to avoid re-filtering every time?

}
},
compactor(cfs, userVersion));
this.journal =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to extract this portion of the patch and get it committed to trunk.

if (index == null)
index = OnDiskIndex.rebuildAndPersist(descriptor, keySupport, metadata.fsyncLimit());

KeyStats.Static<K> keyStats = keyStatsFactory.load(descriptor);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OffsetRangesFactory#load doesn't seem to have any path that would return null. Maybe instead of throwing JournalReadError we should log it and return null?

public void truncateForTesting()
{
ActiveSegment<?, ?> discarding = currentSegment;
if (!discarding.isEmpty()) // if there is no data in the segement then ignore it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, and I know this is not your code, but segement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants