[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. IMPALA-8312 : Alter database operations have race condition This patch fixes a race condition in the alter database implementation in the catalogOpExecutor. The original implementation did a in-place modification of the metastore database object in the Db. This can lead to partially updated database object becoming visible to a reading thread causing potential problems. In order to fix the race, the patch makes a copy of the existing database object, modifies the copy and then atomically switches the actual database object with the modified copy. This is done while holding the metastoreddlLock, and then taking the write lock on the catalog version object which makes it consistent with the other catalog write operations. Added a test which consistently reproduces the race. The test creating many reader threads and a writer thread which continuously keeps changing the owner name and its type by issuing a alter database operation. The test fails without the patch. After the patch the test passes. The race also applies to the alter database set comment operation, although its hard to write a test for that code-path. Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Reviewed-on: http://gerrit.cloudera.org:8080/12789 Reviewed-by: Fredy Wijaya Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java A fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java 4 files changed, 314 insertions(+), 41 deletions(-) Approvals: Fredy Wijaya: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 11 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2593/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 10 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Fri, 29 Mar 2019 18:01:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 10: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3967/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 10 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Fri, 29 Mar 2019 17:29:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 10 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Fri, 29 Mar 2019 17:29:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Vihang Karajgaonkar has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. IMPALA-8312 : Alter database operations have race condition This patch fixes a race condition in the alter database implementation in the catalogOpExecutor. The original implementation did a in-place modification of the metastore database object in the Db. This can lead to partially updated database object becoming visible to a reading thread causing potential problems. In order to fix the race, the patch makes a copy of the existing database object, modifies the copy and then atomically switches the actual database object with the modified copy. This is done while holding the metastoreddlLock, and then taking the write lock on the catalog version object which makes it consistent with the other catalog write operations. Added a test which consistently reproduces the race. The test creating many reader threads and a writer thread which continuously keeps changing the owner name and its type by issuing a alter database operation. The test fails without the patch. After the patch the test passes. The race also applies to the alter database set comment operation, although its hard to write a test for that code-path. Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java A fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java 4 files changed, 314 insertions(+), 41 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/12789/10 -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 10 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 9: (1 comment) Just a nit for constants. I'm ready to give a +2 after the fix. http://gerrit.cloudera.org:8080/#/c/12789/9/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java File fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java: http://gerrit.cloudera.org:8080/#/c/12789/9/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@69 PS9, Line 69: private static int NUM_READERS = 10; : // number of writer threads which change the database. We only need one currently : // since all the alterDatabase calls are serialized by metastoreDdlLock_ in : // CatalogOpExecutor : private static int NUM_WRITERS = 1; nit: add final, these are constants. -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 9 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Fri, 29 Mar 2019 15:58:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 9: Code-Review+1 Fredy, do you have any more comments? -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 9 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Fri, 29 Mar 2019 02:52:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2583/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 9 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Fri, 29 Mar 2019 00:30:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Vihang Karajgaonkar has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. IMPALA-8312 : Alter database operations have race condition This patch fixes a race condition in the alter database implementation in the catalogOpExecutor. The original implementation did a in-place modification of the metastore database object in the Db. This can lead to partially updated database object becoming visible to a reading thread causing potential problems. In order to fix the race, the patch makes a copy of the existing database object, modifies the copy and then atomically switches the actual database object with the modified copy. This is done while holding the metastoreddlLock, and then taking the write lock on the catalog version object which makes it consistent with the other catalog write operations. Added a test which consistently reproduces the race. The test creating many reader threads and a writer thread which continuously keeps changing the owner name and its type by issuing a alter database operation. The test fails without the patch. After the patch the test passes. The race also applies to the alter database set comment operation, although its hard to write a test for that code-path. Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java A fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java 4 files changed, 314 insertions(+), 41 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/12789/9 -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 9 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 8: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/main/java/org/apache/impala/catalog/Db.java@71 PS5, Line 71: final Logger L > I think it will be more invasive and should probably be taken up as a separ wfm. Ya, code is a mess. May be we can do it as a separate patch. http://gerrit.cloudera.org:8080/#/c/12789/8/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: http://gerrit.cloudera.org:8080/#/c/12789/8/fe/src/main/java/org/apache/impala/catalog/Db.java@72 PS8, Line 72: TODO: We should have a consistent synchronization model for Db and Table : //Right now, we synchronize functions and thriftDb_ object in-place and do : //n nit: Prefix lines with spaces // TODO... // Right now... http://gerrit.cloudera.org:8080/#/c/12789/8/fe/src/main/java/org/apache/impala/catalog/Db.java@478 PS8, Line 478: Nullable What does it mean for this to be null? Any specific reason this was added? http://gerrit.cloudera.org:8080/#/c/12789/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/12789/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3125 PS8, Line 3125: dbName I think this can be inferred from msDb. (getName()) -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 8 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 28 Mar 2019 22:14:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2567/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 8 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 27 Mar 2019 23:07:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Vihang Karajgaonkar has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. IMPALA-8312 : Alter database operations have race condition This patch fixes a race condition in the alter database implementation in the catalogOpExecutor. The original implementation did a in-place modification of the metastore database object in the Db. This can lead to partially updated database object becoming visible to a reading thread causing potential problems. In order to fix the race, the patch makes a copy of the existing database object, modifies the copy and then atomically switches the actual database object with the modified copy. This is done while holding the metastoreddlLock, and then taking the write lock on the catalog version object which makes it consistent with the other catalog write operations. Added a test which consistently reproduces the race. The test creating many reader threads and a writer thread which continuously keeps changing the owner name and its type by issuing a alter database operation. The test fails without the patch. After the patch the test passes. The race also applies to the alter database set comment operation, although its hard to write a test for that code-path. Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java A fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java 4 files changed, 314 insertions(+), 41 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/12789/8 -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 8 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Vihang Karajgaonkar has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. IMPALA-8312 : Alter database operations have race condition This patch fixes a race condition in the alter database implementation in the catalogOpExecutor. The original implementation did a in-place modification of the metastore database object in the Db. This can lead to partially updated database object becoming visible to a reading thread causing potential problems. In order to fix the race, the patch makes a copy of the existing database object, modifies the copy and then atomically switches the actual database object with the modified copy. This is done while holding the metastoreddlLock, and then taking the write lock on the catalog version object which makes it consistent with the other catalog write operations. Added a test which consistently reproduces the race. The test creating many reader threads and a writer thread which continuously keeps changing the owner name and its type by issuing a alter database operation. The test fails without the patch. After the patch the test passes. The race also applies to the alter database set comment operation, although its hard to write a test for that code-path. Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java A fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java 4 files changed, 311 insertions(+), 41 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/12789/7 -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 7 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/main/java/org/apache/impala/catalog/Db.java@71 PS5, Line 71: AtomicReference > It seems to be that the synchronization model for Db is different than of T Is it too much code for fixing the synchronization model for Db objects? (Make it consistent with how we update/read tables). -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 5 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 21 Mar 2019 22:34:34 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2494/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 6 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 21 Mar 2019 00:52:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Vihang Karajgaonkar has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. IMPALA-8312 : Alter database operations have race condition This patch fixes a race condition in the alter database implementation in the catalogOpExecutor. The original implementation did a in-place modification of the metastore database object in the Db. This can lead to partially updated database object becoming visible to a reading thread causing potential problems. In order to fix the race, the patch makes a copy of the existing database object, modifies the copy and then atomically switches the actual database object with the modified copy. This is done while holding the metastoreddlLock, and then taking the write lock on the catalog version object which makes it consistent with the other catalog write operations. Added a test which consistently reproduces the race. The test creating many reader threads and a writer thread which continuously keeps changing the owner name and its type by issuing a alter database operation. The test fails without the patch. After the patch the test passes. The race also applies to the alter database set comment operation, although its hard to write a test for that code-path. Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java A fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java 4 files changed, 300 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/12789/6 -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 6 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/12789 )
Change subject: IMPALA-8312 : Alter database operations have race condition
..
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12789/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12789/5//COMMIT_MSG@12
PS5, Line 12: partially updated database object becoming visible to a reading
thread causing
nit: line overflows
http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/main/java/org/apache/impala/catalog/Db.java@71
PS5, Line 71: AtomicReference
Is there any advantage in using AtomicReference when any changes to the parent
Db only occur under an exclusive version lock?
http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java
File fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java:
http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@80
PS5, Line 80: public static void setUpTest() throws ImpalaException {
Add method doc?
http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@131
PS5, Line 131: ReaderTask
ValidateDbOwnerTask or something?
http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@149
PS5, Line 149: WriterTask
Better name?
--
To view, visit http://gerrit.cloudera.org:8080/12789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc
Gerrit-Change-Number: 12789
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 20 Mar 2019 21:18:24 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java File fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java: http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@66 PS5, Line 66: private static int numReaders_ = 10; : // number of writer threads which change the database. We only need one currently : // since all the alterDatabase calls are serialized by metastoreDdlLock_ in : // CatalogOpExecutor : private static int numWriters_ = 1; Can be static final. Use upper case for static final variable names. http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@72 PS5, Line 72: // barrier to make sure that readers and writers start at the same time : private static final CyclicBarrier barrier_ = : new CyclicBarrier(numReaders_ + numWriters_); : // toggle switch to change the database owner from user_1 to user_2 in each : // consecutive alter database call : private static final AtomicBoolean toggler_ = new AtomicBoolean(false); I feel like all these variables should not be static especially since we may add another test in the future and need to reset the toggler_, etc. http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@132 PS5, Line 132: nit: remove new line http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@150 PS5, Line 150: nit: remove new line http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@224 PS5, Line 224: throw new Exception should we just use fail() instead of throwing an exception? -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 5 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:46:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2465/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 5 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 19 Mar 2019 18:34:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Vihang Karajgaonkar has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. IMPALA-8312 : Alter database operations have race condition This patch fixes a race condition in the alter database implementation in the catalogOpExecutor. The original implementation did a in-place modification of the metastore database object in the Db. This can lead to partially updated database object becoming visible to a reading thread causing potential problems. In order to fix the race, the patch makes a copy of the existing database object, modifies the copy and then atomically switches the actual database object with the modified copy. This is done while holding the metastoreddlLock, and then taking the write lock on the catalog version object which makes it consistent with the other catalog write operations. Added a test which consistently reproduces the race. The test creating many reader threads and a writer thread which continuously keeps changing the owner name and its type by issuing a alter database operation. The test fails without the patch. After the patch the test passes. The race also applies to the alter database set comment operation, although its hard to write a test for that code-path. Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java A fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java 4 files changed, 288 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/12789/5 -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 5 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 4: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/2460/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 4 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 19 Mar 2019 01:42:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Vihang Karajgaonkar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12789 Change subject: IMPALA-8312 : Alter database operations have race condition .. IMPALA-8312 : Alter database operations have race condition This patch fixes a race condition in the alter database implementation in the catalogOpExecutor. The original implementation did a in-place modification of the metastore database object in the Db. This can lead to partially updated database object becoming visible to a reading thread causing potential problems. In order to fix the race, the patch makes a copy of the existing database object, modifies the copy and then atomically switches the actual database object with the modified copy. This is done while holding the metastoreddlLock, and then taking the write lock on the catalog version object which makes it consistent with the other catalog write operations. Added a test which consistently reproduces the race. The test creating many reader threads and a writer thread which continuously keeps changing the owner name and its type by issuing a alter database operation. The test fails without the patch. After the patch the test passes. The race also applies to the alter database set comment operation, although its hard to write a test for that code-path. Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java A fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java 4 files changed, 288 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/12789/4 -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 4 Gerrit-Owner: Vihang Karajgaonkar
