CASSANALYTICS-104: Eliminate redundant filesystem lookups in SSTable streaming - #239
CASSANALYTICS-104: Eliminate redundant filesystem lookups in SSTable streaming#239lukasz-antoniak wants to merge 3 commits into
Conversation
3b0267c to
d49c9b5
Compare
c3d5f0f to
93f4906
Compare
yifan-c
left a comment
yifan-c
left a comment
There was a problem hiding this comment.
Please update the CHANGES.txt
| .map(StackTraceElement::toString) | ||
| .collect(Collectors.joining("\n")); | ||
| String errorMessage = exception.getClass().getName() + ": " + exception.getMessage() | ||
| + "\n" + String.join("\n", stackTrace); |
There was a problem hiding this comment.
String.join("\n", stackTrace) is equivalent to just stackTrace, right?
The stacktrace are already aggregated in the statement above.
| // 4. remove the sstables once sent | ||
| Map<Path, Digest> fileDigests = sstableWriter.prepareSStablesToSend(writerContext, sstables); | ||
| SortedSSTableWriter.PreparedSSTables preparedSSTables = sstableWriter.prepareSStablesToSend(writerContext, sstables); | ||
| // retain only the SSTable data components |
There was a problem hiding this comment.
Please remove this comment, it is stale with the removal of .filter
| { | ||
| for (Path dataFile : dataFileStream) | ||
| { | ||
| if (isFileStreamed(dataFile)) |
There was a problem hiding this comment.
StreamSession.isFileStreamed is now dead. Can you remove the code in the base class?
There was a problem hiding this comment.
The effect of this method has no use. It can be deleted, along with isFileStreamed
| { | ||
| for (Path componentFile : componentFileStream) | ||
| // send data component the last | ||
| if (preparedSSTable.dataFile().equals(componentFile)) |
There was a problem hiding this comment.
Can dataFile() return null?
There was a problem hiding this comment.
Theoretically it could, but only if data file was not generated during sstable creation (e.g. remainingSSTablesAfterClose contained some sstable files, but without Data.db). I have replaced the operands in equality check, and added preconditions check.
| */ | ||
| public static class PreparedSSTables | ||
| { | ||
| private static final PreparedSSTables EMPTY = new PreparedSSTables(); |
There was a problem hiding this comment.
EMPTY is mutable. Caller can call addIfAbsent on it and mess up the state.
There was a problem hiding this comment.
Can you recheck now? I did not think of an easier way to fix this issue.
| public PreparedSSTable addIfAbsent(Path path) | ||
| { | ||
| String baseName = SSTables.getSSTableDescriptor(path).baseFilename; | ||
| return sstables.computeIfAbsent(baseName, (__) -> new PreparedSSTable()); |
There was a problem hiding this comment.
Based on the impl, getOrPrepareSSTable method name fits better.
2 participants
Fixes CASSANALYTICS-104.