blambov commented on code in PR #2689:
URL: https://github.com/apache/cassandra/pull/2689#discussion_r1328721589
##########
src/java/org/apache/cassandra/io/sstable/format/bti/BtiTableWriter.java:
##########
@@ -358,25 +315,52 @@ public MmappedRegionsCache getMmappedRegionsCache()
}
@Override
- public SequentialWriter getDataWriter()
+ protected SequentialWriter openDataWriter(OperationType operationType)
+ {
+ checkState(dataWriter == null, "Data writer has been already
opened.");
+ dataWriter = DataComponent.buildWriter(descriptor,
+
getTableMetadataRef().getLocal(),
+
getIOOptions().writerOptions,
+ getMetadataCollector(),
+ operationType,
+
getIOOptions().flushCompression);
+ return dataWriter;
+ }
+
+ SequentialWriter getDataWriter()
{
return ensuringInBuildInternalContext(dataWriter);
}
@Override
- public BtiFormatPartitionWriter getPartitionWriter()
+ protected BtiFormatPartitionWriter openPartitionWriter()
{
+ checkState(partitionWriter == null, "Partition writer has been
already opened.");
+ partitionWriter = new
BtiFormatPartitionWriter(getSerializationHeader(),
+
getTableMetadataRef().getLocal().comparator,
+ getDataWriter(),
+
getIndexWriter().rowIndexWriter,
+ descriptor.version);
+
return ensuringInBuildInternalContext(partitionWriter);
Review Comment:
This call is no longer necessary.
##########
src/java/org/apache/cassandra/io/sstable/format/bti/BtiTableWriter.java:
##########
@@ -358,25 +315,52 @@ public MmappedRegionsCache getMmappedRegionsCache()
}
@Override
- public SequentialWriter getDataWriter()
+ protected SequentialWriter openDataWriter(OperationType operationType)
+ {
+ checkState(dataWriter == null, "Data writer has been already
opened.");
+ dataWriter = DataComponent.buildWriter(descriptor,
+
getTableMetadataRef().getLocal(),
+
getIOOptions().writerOptions,
+ getMetadataCollector(),
+ operationType,
+
getIOOptions().flushCompression);
+ return dataWriter;
+ }
+
+ SequentialWriter getDataWriter()
{
return ensuringInBuildInternalContext(dataWriter);
}
@Override
- public BtiFormatPartitionWriter getPartitionWriter()
+ protected BtiFormatPartitionWriter openPartitionWriter()
Review Comment:
I see the problem now... it appears to me that it would be safer to pass
index and data writers as arguments, but leave as is if you prefer.
##########
src/java/org/apache/cassandra/io/sstable/format/bti/BtiTableWriter.java:
##########
@@ -358,25 +315,52 @@ public MmappedRegionsCache getMmappedRegionsCache()
}
@Override
- public SequentialWriter getDataWriter()
+ protected SequentialWriter openDataWriter(OperationType operationType)
+ {
+ checkState(dataWriter == null, "Data writer has been already
opened.");
+ dataWriter = DataComponent.buildWriter(descriptor,
+
getTableMetadataRef().getLocal(),
+
getIOOptions().writerOptions,
+ getMetadataCollector(),
+ operationType,
+
getIOOptions().flushCompression);
+ return dataWriter;
+ }
+
+ SequentialWriter getDataWriter()
{
return ensuringInBuildInternalContext(dataWriter);
}
@Override
- public BtiFormatPartitionWriter getPartitionWriter()
+ protected BtiFormatPartitionWriter openPartitionWriter()
{
+ checkState(partitionWriter == null, "Partition writer has been
already opened.");
+ partitionWriter = new
BtiFormatPartitionWriter(getSerializationHeader(),
Review Comment:
Note: Generally, if the only purpose of this field is to forbid second
opening, I would replace it with a boolean, so that there is no chance that
this reference causes the garbage collector to keep the object.
This should not be a real problem here as the builder reference is not
something that is kept around.
##########
src/java/org/apache/cassandra/io/sstable/format/bti/BtiTableWriter.java:
##########
@@ -358,25 +315,52 @@ public MmappedRegionsCache getMmappedRegionsCache()
}
@Override
- public SequentialWriter getDataWriter()
+ protected SequentialWriter openDataWriter(OperationType operationType)
+ {
+ checkState(dataWriter == null, "Data writer has been already
opened.");
+ dataWriter = DataComponent.buildWriter(descriptor,
+
getTableMetadataRef().getLocal(),
+
getIOOptions().writerOptions,
+ getMetadataCollector(),
+ operationType,
+
getIOOptions().flushCompression);
+ return dataWriter;
+ }
+
+ SequentialWriter getDataWriter()
{
return ensuringInBuildInternalContext(dataWriter);
}
@Override
- public BtiFormatPartitionWriter getPartitionWriter()
+ protected BtiFormatPartitionWriter openPartitionWriter()
{
+ checkState(partitionWriter == null, "Partition writer has been
already opened.");
+ partitionWriter = new
BtiFormatPartitionWriter(getSerializationHeader(),
+
getTableMetadataRef().getLocal().comparator,
+ getDataWriter(),
+
getIndexWriter().rowIndexWriter,
+ descriptor.version);
+
return ensuringInBuildInternalContext(partitionWriter);
}
- public IndexWriter getIndexWriter()
+ @Override
+ protected IndexWriter openIndexWriter()
+ {
+ checkState(indexWriter == null, "Index writer has been already
opened.");
+ indexWriter = new IndexWriter(this);
+ return indexWriter;
+ }
+
+ IndexWriter getIndexWriter()
{
return ensuringInBuildInternalContext(indexWriter);
}
private <T> T ensuringInBuildInternalContext(T value)
{
- Preconditions.checkState(value != null, "This getter can be used
only during the lifetime of the sstable constructor. Do not use it directly.");
+ checkState(value != null, "This getter can be used only during the
lifetime of the sstable constructor. Do not use it directly.");
Review Comment:
It appears that we will most likely get this if `openPartitionWriter` is
called before `openDataWriter` or `openIndexWriter`. Perhaps change the message
to say it?
--
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]
