pkolaczk commented on code in PR #2420:
URL: https://github.com/apache/cassandra/pull/2420#discussion_r1236617154
##########
src/java/org/apache/cassandra/db/streaming/ComponentManifest.java:
##########
@@ -52,13 +53,17 @@ public ComponentManifest(Map<Component, Long> components)
}
@VisibleForTesting
- public static ComponentManifest create(Descriptor descriptor)
+ public static ComponentManifest create(SSTable sstable)
{
- LinkedHashMap<Component, Long> components = new
LinkedHashMap<>(descriptor.getFormat().streamingComponents().size());
+ Set<Component> streamingComponents = sstable.getComponents();
+ LinkedHashMap<Component, Long> components = new
LinkedHashMap<>(streamingComponents.size());
- for (Component component :
descriptor.getFormat().streamingComponents())
+ for (Component component : streamingComponents)
{
- File file = descriptor.fileFor(component);
+ if (component == SSTableFormat.Components.TOC)
Review Comment:
First, I like the idea of having `getStreamingComponents` method on the
sstable. This looks like a better place to tell which components are eligible
for streaming.
However, even with such method, we still have a "responsibility leak" from
SAI here.
```
c.type == Components.Types.SAI
```
Why should SSTable code know anything about SAI? Why should it assume all
SAI components should be streamed? IMHO this is something that should be
decided by the SAI code (or whoever owns the component), not by sstable code.
SAI already controls the type of the component, so I guess the ideal-world
solution would be to have this information associated directly with the type so
instead of special-casing SAI vs core components, we could write the condition
like this:
```
return components.stream()
.filter(c -> c.type.shouldBeStreamed)
.collect(Collectors.toSet());
```
WDYT?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
