Skip to content

Conversation

@smiklosovic
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

Copy link
Contributor

@yifan-c yifan-c left a comment

Choose a reason for hiding this comment

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

Looks good overall. I have some nits and questions.

}
else
{
useOwner = owner;
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, the input owner is always null in the code base. Why do we ever need this parameter? Maybe @jacek-lewandowski can share some context. Looks like the parameter is added in the SSTable format API patch.
In case it is consumed by some external system, should it be more clear if the owner is assigned when creating the AbstractSSTableSimpleWriter? So that we can avoid the fallback logic 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.

I added constructors accepting SSTable.Owner in both writers as suggested above. I would still keep this logic as is for integrations using this differently so it would be still compatible.

@smiklosovic smiklosovic requested a review from yifan-c November 3, 2025 13:45
@smiklosovic smiklosovic force-pushed the CASSANDRA-20938 branch 2 times, most recently from bb22bfe to ccd2d49 Compare November 4, 2025 09:46
patch by Stefan Miklosovic; reviewed by Yifan Cai for CASSANDRA-20938
@smiklosovic smiklosovic merged commit 1a96b88 into apache:trunk Nov 5, 2025
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