jyothsnakonisa commented on code in PR #4523:
URL: https://github.com/apache/cassandra/pull/4523#discussion_r2638942594
##########
src/java/org/apache/cassandra/db/compression/ZstdDictionaryTrainer.java:
##########
@@ -334,7 +342,8 @@ public void reset()
{
totalSampleSize.set(0);
sampleCount.set(0);
- zstdTrainer = new ZstdDictTrainer(config.maxTotalSampleSize,
config.maxDictionarySize, compressionLevel);
+ zstdTrainer = new
ZstdDictTrainer(trainingConfig.maxTotalSampleSize,
trainingConfig.maxDictionarySize, compressionLevel);
+ config = null;
Review Comment:
Why is `trainingConfig` that is passed as parameter not being assigned to
config here? when config is set to null here and then set `trainingConfig`
after reset is called, there will be a small window where the config is null
before setting to `trainingConfig`, don't you think it is better to set the
config here instead of setting it to null and assigning `trainingConfig` later?
##########
src/java/org/apache/cassandra/db/compression/ZstdDictionaryTrainer.java:
##########
@@ -50,13 +50,13 @@ public class ZstdDictionaryTrainer implements
ICompressionDictionaryTrainer
private final String keyspaceName;
private final String tableName;
- private final CompressionDictionaryTrainingConfig config;
+ private volatile CompressionDictionaryTrainingConfig config;
Review Comment:
Since config can be reset to null, can you add a null check before using it?
I think you should add the check in `buildNotReadyMessage()` method and please
check if it should be added anywhere else.
##########
src/java/org/apache/cassandra/db/compression/ZstdDictionaryTrainer.java:
##########
@@ -68,24 +68,30 @@ public class ZstdDictionaryTrainer implements
ICompressionDictionaryTrainer
private volatile TrainingStatus currentTrainingStatus;
private volatile String failureMessage;
- public ZstdDictionaryTrainer(String keyspaceName, String tableName,
- CompressionDictionaryTrainingConfig config,
- int compressionLevel)
+ public ZstdDictionaryTrainer(String keyspaceName, String tableName, int
compressionLevel)
+ {
+ this(keyspaceName,
+ tableName,
+ compressionLevel,
+
DatabaseDescriptor.getCompressionDictionaryTrainingSamplingRate());
+ }
+
+ @VisibleForTesting
+ public ZstdDictionaryTrainer(String keyspaceName, String tableName, int
compressionLevel, float samplingRate)
{
this.keyspaceName = keyspaceName;
this.tableName = tableName;
- this.config = config;
this.totalSampleSize = new AtomicLong(0);
this.sampleCount = new AtomicLong(0);
this.compressionLevel = compressionLevel;
- this.samplingRate = config.samplingRate;
+ this.samplingRate = samplingRate;
this.currentTrainingStatus = TrainingStatus.NOT_STARTED;
}
@Override
public boolean shouldSample()
{
- return zstdTrainer != null &&
ThreadLocalRandom.current().nextInt(samplingRate) == 0;
+ return zstdTrainer != null && ThreadLocalRandom.current().nextFloat()
< samplingRate;
Review Comment:
Can you make `zstdTrainer` volatile? It is updated using synchronized but
while reading there is no synchronized. We can still read stale value even
after synchronized update.
--
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]
