HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. #6171
Conversation
…ctionLogTable and DAG.
| @@ -587,7 +587,7 @@ void addToCompactionLogTable(CompactionLogEntry compactionLogEntry) { | |||
| * Check if there is any in_progress tarball creation request and wait till | |||
| * all tarball creation finish, and it gets notified. | |||
| */ | |||
| private void waitForTarballCreation() { | |||
| private synchronized void waitForTarballCreation() { | |||
There was a problem hiding this comment.
How would this change help in anyway? I don't think rocksdb will do multiple compaction in parallel. onCompactionCompleted method will commit the compaction.
There was a problem hiding this comment.
I believe we would need a lock on the tarballRequestCount when incrementing and decrementing request and add the compaction log entry within the lock.
There was a problem hiding this comment.
waitForTarballCreation wouldn't really ensure the count would be zero after the method call. There could be a race condition here.
There was a problem hiding this comment.
tarballRequestCount is an AtomicInteger so the incrementing and decrementing it would be atomic.
waitForTarballCreation is synchronized because wait/notify are supposed to be allowed on the same object's lock otherwise it throws IllegalMonitorStateException. Doc: wait/notifyAll.
More in discussion on jira: HDDS-9039 why we are adding the test this way.
There was a problem hiding this comment.
What I didn't understand is getCheckpoint() method is not taking a synchronized lock. So the question if waitForTarballCreation() passes.
can still run. Thus isn't there a race condition here? So while compaction is running we still end up starting a tarballing for the checkpoint.There was a problem hiding this comment.
I think you are right if we are appending compaction log entries to a txt file but we don't use that anymore and add entries to compactionLog columnFamily.
I gave it more thought and I think this locking is not needed anymore because we use RocksDB column family for compaction now. So compaction entry will be either present in the table or not in the snapshot of the ActiveFS depending on the order of compaction entry append and checkpoint creation. Hence we can simply remove this locking code and just rely on RocksDB.
Original discussion to add lock: #4680 (comment)
There was a problem hiding this comment.
yeah i agree we don't need the locks since we are writing it into a rocksdb table. This is fine as long as we are using rocksdb checkpoints to take a checkpoint of the rocksdb.
|
Closing it in favor of #6552 |
What changes were proposed in this pull request?
There are two major changes in this PR.
compactionLogTableand DAG while tarball creation is in progress.waitForTarballCreationsynchronized function so that wait and notify threads are on a same object's monitor.What is the link to the Apache JIRA
HDDS-9039
How was this patch tested?
Ran newly added test locally and fork branch.