You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "hemantk-12 (via GitHub)" <gi...@apache.org> on 2024/02/05 17:28:51 UTC
[PR] HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. [ozone]
hemantk-12 opened a new pull request, #6171:
URL: https://github.com/apache/ozone/pull/6171
## What changes were proposed in this pull request?
There are two major changes in this PR.
1. Added a new test to validate that no new compaction log entry is added to `compactionLogTable` and DAG while tarball creation is in progress.
2. Made `waitForTarballCreation` synchronized 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #6171:
URL: https://github.com/apache/ozone/pull/6171#issuecomment-2062589569
Closing it in favor of https://github.com/apache/ozone/pull/6552
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #6171:
URL: https://github.com/apache/ozone/pull/6171#discussion_r1568195432
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -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() {
Review 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 or compaction entry append and checkpoint creation. Hence we can simply remove this locking code and just rely on RocksDB.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #6171:
URL: https://github.com/apache/ozone/pull/6171#discussion_r1480430031
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -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() {
Review 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.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -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() {
Review Comment:
waitForTarballCreation wouldn't really ensure the count would be zero after the method call. There could be a race condition here.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -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() {
Review 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #6171:
URL: https://github.com/apache/ozone/pull/6171#discussion_r1569262977
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -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() {
Review 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #6171:
URL: https://github.com/apache/ozone/pull/6171#discussion_r1480488656
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -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() {
Review 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](https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#wait--)/[notifyAll](https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#notifyAll--).
More in discussion on jira: HDDS-9039 why we are adding the test this way.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 closed pull request #6171: HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress.
URL: https://github.com/apache/ozone/pull/6171
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #6171:
URL: https://github.com/apache/ozone/pull/6171#discussion_r1568195432
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -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() {
Review 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: https://github.com/apache/ozone/pull/4680#issuecomment-1553512855
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #6171:
URL: https://github.com/apache/ozone/pull/6171#discussion_r1568195432
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -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() {
Review 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #6171:
URL: https://github.com/apache/ozone/pull/6171#discussion_r1568100595
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -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() {
Review Comment:
What I didn't understand is getCheckpoint() method is not taking a synchronized lock. So the question if waitForTarballCreation() passes. https://github.com/apache/ozone/blob/2c0580dbb2edeb94ec263aa44fd1e22cff838dcc/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java#L248 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org