Skip to content

Replay nondurable #4264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

belliottsmith
Copy link
Contributor

Thanks for sending a pull request! Here are some tips if you're new here:

  • Ensure you have added or run the appropriate tests for your PR.
  • Be sure to keep the PR description updated to reflect all changes.
  • Write your PR title to summarize what this PR proposes.
  • If possible, provide a concise example to reproduce the issue for a faster review.
  • Read our contributor guidelines
  • If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

@belliottsmith belliottsmith force-pushed the replay-nondurable branch 2 times, most recently from c6ba404 to 831de21 Compare July 22, 2025 15:24

public class AccordDataStore implements DataStore
{
private static final Logger logger = LoggerFactory.getLogger(AccordDataStore.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you briefly document durability semantics here? In other words, the fact that sync point is executed after we have flushed meltable?

@@ -78,35 +68,42 @@ public boolean hasTasks()
return tasks + executor.getActiveTaskCount() + executor.getPendingTaskCount() > 0;
}

@Override
void preUnlockCaches()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe "before" instead of "pre"?

@@ -492,6 +492,8 @@ void initParent()
@Override
public void close()
{
// TODO (required): validate this in IntervalBTreeTest
Copy link
Contributor

Choose a reason for hiding this comment

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

🫡

public static class UniqueSave
{
@Nullable List<Runnable> onSuccess;
void onSuccess(Runnable onSuccess)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like onSuccess can be called from multiple threads (via shared executor submission), should we use volatile and some concurrent structure here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's all done under mutual exclusion while holding the executor lock.


T listener;
if (null == (listener = (T)onFlush.get(key)))
onFlush = ImmutableMap.<Object, Consumer<TableMetadata>>builder().putAll(onFlush).put(key, listener = factory.get()).build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind if we not used assignments inside statements (it is just so easy to miss them), and did something like this:

      T listener = (T) onFlush.get(key);
      if (listener == null)
      {
          listener = factory.get();
          onFlush = ImmutableMap.<Object, Consumer<TableMetadata>>builder()
                               .putAll(onFlush)
                               .put(key, listener)
                               .build();
      }
      return listener;

Patch by Alex Petrov; reviewed by Benedict Elliott Smith for CASSANDRA-20765
Patch by Alex Petrov; reviewed by Benedict Elliott Smith for CASSANDRA-20767
…tore and data store

Also fix:
 - Limit truncation to TruncatedApplyWithOutcome until data is persisted durably to local store
 - IntervalUpdater not invoking super.close()
 - Do not invoke preRunExclusive without holding lock
 - IntervalUpdater not correctly initialise BranchBuilder.inUse
 - AccordExecutor should notify if more work on unlock of caches
 - Relax paranoid CFK validation during restart
Also improve:
 - Flush logs before System.exit
 - Start/stop progress log explicitly
 - Limit progress log concurrency
 - Clear heavy fields in some messages once processed

patch by Benedict; reviewed by Alex Petrov for CASSANDRA-20780
@belliottsmith belliottsmith force-pushed the trunk branch 3 times, most recently from df3eb40 to 54e39a9 Compare July 23, 2025 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants