Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CASSANDRA-18345 Stream all components registered by an SSTable #2420

Closed
wants to merge 17 commits into from
Closed

CASSANDRA-18345 Stream all components registered by an SSTable #2420

wants to merge 17 commits into from

Conversation

pkolaczk
Copy link
Contributor

Custom indexes may register custom components.
We need to include them in streaming as well.

@pkolaczk pkolaczk marked this pull request as ready for review June 15, 2023 14:42
@pkolaczk pkolaczk marked this pull request as draft June 16, 2023 09:41
@maedhroz
Copy link
Contributor

nit: There are some unused import errors on J11 checkstyle

@pkolaczk
Stream all components registered by an sstable
c838ece 
Indexes may register custom components.
We need to include them in streaming to avoid costly
building of indexes on the receiving side.

In order to make this work,
zero copy streaming had to be modified to index its writers
by component name instead of component type because
component types are not unique - e.g. many index components
are of type CUSTOM.
@pkolaczk pkolaczk marked this pull request as ready for review June 19, 2023 12:08
maedhroz
maedhroz reviewed Jun 20, 2023
View reviewed changes
test/distributed/org/apache/cassandra/distributed/test/sai/IndexStreamingTest.java
"CREATE TABLE %s.test (pk int PRIMARY KEY, v text, b blob) WITH compression = { 'enabled' : false };"
));
cluster.schemaChange(withKeyspace(
"CREATE CUSTOM INDEX ON %s.test(v) USING 'StorageAttachedIndex';"
Copy link
Contributor

@maedhroz maedhroz Jun 20, 2023 •
edited

Choose a reason for hiding this comment

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

Once numerics support merges, I guess we'll want to parameterize this test further to use those. The number of expected index components might not actually change though. CC @mike-tr-adamson

I'm not sure if that means we should merge this first and adjust inside a rebased CASSANDRA-18067 or the other way around.

maedhroz
maedhroz requested changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@maedhroz maedhroz left a comment •
edited

Choose a reason for hiding this comment

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

The approach looks fine here. There are just a few things we should at least double check in terms of how we generate and validate the components we want to stream (...and we have to decide whether to merge this now and make the numerics patch add some testing or wait for numerics and test in this patch).

@pkolaczk
Copy link
Contributor Author

I addressed all review comments, except the part about introducing the SAI type for SAI components, because IMHO that needs a wider discussion.

@pkolaczk
Allow component types to decide whether they need to be streamed
e4e746e 
Instead of hardcoding a collection of streamable components,
a flag `streamable` is added to the component type.

This allows custom components (e.g. SAI components) to decide whether
they should be streamed.
@pkolaczk
Remove SSTableFormat#streamingComponents
123b3f7 
We don't need it because we can check streamability by inspecting
component's streamable field directly.
adelapena
adelapena reviewed Jun 26, 2023
View reviewed changes
src/java/org/apache/cassandra/io/sstable/format/big/BigFormat.java Show resolved Hide resolved
src/java/org/apache/cassandra/io/sstable/Component.java Outdated Show resolved Hide resolved
src/java/org/apache/cassandra/io/sstable/Component.java Outdated Show resolved Hide resolved
test/distributed/org/apache/cassandra/distributed/test/sai/IndexStreamingTest.java Outdated Show resolved Hide resolved
test/distributed/org/apache/cassandra/distributed/test/sai/IndexStreamingTest.java Outdated Show resolved Hide resolved
test/distributed/org/apache/cassandra/distributed/test/sai/IndexStreamingTest.java Outdated Show resolved Hide resolved
test/distributed/org/apache/cassandra/distributed/test/sai/IndexStreamingTest.java Outdated Show resolved Hide resolved
test/distributed/org/apache/cassandra/distributed/test/sai/IndexStreamingTest.java Outdated Show resolved Hide resolved
src/java/org/apache/cassandra/io/sstable/SSTableZeroCopyWriter.java Show resolved Hide resolved
@pkolaczk
Use distinct component types for SAI components
ad042fb 
Don't use CUSTOM component.
Instead, create separate component type for each SAI index component type.
maedhroz
maedhroz approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@maedhroz maedhroz left a comment

Choose a reason for hiding this comment

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

Made a few more nits, but this looks pretty nice now.

Again, we'll have to modify/add some things here to deal w/ the numerics patch when it's ready (or in that patch if this merges), but that shouldn't be hard.

pkolaczk and others added 10 commits June 27, 2023 10:23
@pkolaczk @adelapena
Split long lines
b1e09ec 
Co-authored-by: Andrés de la Peña <adelapena@users.noreply.github.com>
@pkolaczk @adelapena
Update test/distributed/org/apache/cassandra/distributed/test/sai/Ind…
0068ad2 
…exStreamingTest.java

Co-authored-by: Andrés de la Peña <adelapena@users.noreply.github.com>
@pkolaczk @adelapena
Use broadcastAddress instead of hardcoded IP
8664c94 
Co-authored-by: Andrés de la Peña <adelapena@users.noreply.github.com>
@pkolaczk @adelapena
Split
0e426f1 
Co-authored-by: Andrés de la Peña <adelapena@users.noreply.github.com>
@pkolaczk @maedhroz
Fix assertion message
46cc84e 
Co-authored-by: Caleb Rackliffe <maedhroz@users.noreply.github.com>
@pkolaczk
Add streamable param to code samples in SSTable_API.md
f742c5e
@pkolaczk
Rename isStreamable -> streamable
53d40ca
@pkolaczk
Move comment to a better place
f4d03b0
@pkolaczk
Fix formatting
1fba9bb
@pkolaczk
Improve SAI component regex
82df4e1
@maedhroz maedhroz changed the title Stream all components registered by an sstable CASSANDRA-18345 Stream all components registered by an SSTable Jun 27, 2023
@pkolaczk
Copy link
Contributor Author

CI run: https://app.circleci.com/pipelines/github/pkolaczk/cassandra/16/workflows/41cc8da8-cf2b-4985-be0c-cf5ffd9598d8

maedhroz
maedhroz reviewed Jun 28, 2023
View reviewed changes
src/java/org/apache/cassandra/io/sstable/SSTableZeroCopyWriter.java
{
logger.info("Writing component {} to {} length {}", type, componentWriters.get(type).getPath(), prettyPrintMemory(size));
@SuppressWarnings({"resource", "RedundantSuppression"}) // all writers are closed in close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious...can individual writers fail and cause some writers not to be closed? (above, on 197)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if that's the case, we could use FBUtitilites#closeAll at SSTableZeroCopyWriter#close. But we are not modifying that here, so I think it can be done on a separate ticket.

@maedhroz
Copy link
Contributor

Committed as 5d3f257

@maedhroz maedhroz closed this Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maedhroz maedhroz maedhroz approved these changes

@adelapena adelapena adelapena approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@pkolaczk @maedhroz @adelapena