jyothsnakonisa commented on code in PR #4564:
URL: https://github.com/apache/cassandra/pull/4564#discussion_r2710136387
##########
src/java/org/apache/cassandra/io/sstable/AbstractSSTableSimpleWriter.java:
##########
@@ -147,13 +147,13 @@ protected SSTableTxnWriter createWriter(SSTable.Owner
owner) throws IOException
effectiveOwner);
}
- private static Descriptor createDescriptor(File directory, final String
keyspace, final String columnFamily, final SSTableFormat<?, ?> fmt) throws
IOException
+ private Descriptor createDescriptor(File directory, final String keyspace,
final String columnFamily, final SSTableFormat<?, ?> fmt) throws IOException
{
SSTableId nextGen = getNextId(directory, columnFamily);
return new Descriptor(directory, keyspace, columnFamily, nextGen, fmt);
}
- private static SSTableId getNextId(File directory, final String
columnFamily) throws IOException
+ private SSTableId getNextId(File directory, final String columnFamily)
throws IOException
Review Comment:
Not a change that you made but In this method, can you please avoid doing
`try (Stream<Path> existingPaths = Files.list(directory.toPath()))` in the
while loop for CAS to avoid doing full directory scan on each retry for CAS?
##########
src/java/org/apache/cassandra/io/sstable/AbstractSSTableSimpleWriter.java:
##########
@@ -53,7 +53,7 @@ public abstract class AbstractSSTableSimpleWriter implements
Closeable
protected final TableMetadataRef metadata;
protected final RegularAndStaticColumns columns;
protected SSTableFormat<?, ?> format =
DatabaseDescriptor.getSelectedSSTableFormat();
- protected static final AtomicReference<SSTableId> id = new
AtomicReference<>(SSTableIdFactory.instance.defaultBuilder().generator(Stream.empty()).get());
+ protected final AtomicReference<SSTableId> id = new
AtomicReference<>(SSTableIdFactory.instance.defaultBuilder().generator(Stream.empty()).get());
Review Comment:
Since `id` is made non static, can you add documentation about having one
writer per one sstable? also, if that is the case, this class might be used
only by one thread and you might not need thread safety related stuff here.
##########
src/java/org/apache/cassandra/io/sstable/SequenceBasedSSTableId.java:
##########
@@ -115,20 +115,20 @@ public boolean isUniqueIdentifier(String str)
@Override
public boolean isUniqueIdentifier(ByteBuffer bytes)
{
- return bytes != null && bytes.remaining() == Integer.BYTES &&
bytes.getInt(0) >= 0;
+ return bytes != null && bytes.remaining() == Long.BYTES &&
bytes.getLong(0) >= 0;
Review Comment:
When older SStables are read with this new version of cassandra with these
changes, `bytes.remaining() == Long.BYTES` will be false and this method will
return false.
You might want to think about backward compatibility issues, also it would
be nice if you can add tests around those scenarios.
--
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]
