[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/12938 )
Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid
incorrect deletion of Database objects.
..
Patch Set 12: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12938/12//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12938/12//COMMIT_MSG@7
PS12, Line 7: Check CREATION_TIME in event
: processor to avoid incorrect deletion of Database objects.
wrap to single line? Something like "consider ctime while processing database
events"...
http://gerrit.cloudera.org:8080/#/c/12938/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12938/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1546
PS12, Line 1546: if (catalogDb == null) {
: return null;
: }
nit: single line
http://gerrit.cloudera.org:8080/#/c/12938/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1552
PS12, Line 1552: this
nit: not needed
--
To view, visit http://gerrit.cloudera.org:8080/12938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c
Gerrit-Change-Number: 12938
Gerrit-PatchSet: 12
Gerrit-Owner: Bharath Krishna
Gerrit-Reviewer: Anurag Mantripragada
Gerrit-Reviewer: Bharath Krishna
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Mon, 15 Apr 2019 22:26:32 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. Patch Set 12: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2782/ : 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/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 12 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Krishna Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 15 Apr 2019 18:58:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Bharath Krishna has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. Process the drop database event only if the CREATION_TIME of the catalog's database object is lesser than or equal to that of the database object present in the notification event. If the CREATION_TIME in the notification event object is lesser than the catalog's DB object, it means that the Database object present in the catalog is the latest and we can skip the event instead. Testing : - Added unit tests in MetastoreEventsProcessorTest. - Enabled testCreateDropCreateDatabaseFromImpala as we now have CREATION_TIME in the notification events. Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java 3 files changed, 131 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/12938/12 -- To view, visit http://gerrit.cloudera.org:8080/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 12 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Krishna Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/12938 )
Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid
incorrect deletion of Database objects.
..
Patch Set 10:
(13 comments)
A bunch of nits but otherwise lgtm. Can +2 once those are fixed.
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1536
PS10, Line 1536:
..does not exist or not removed because ctime matches..
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1539
PS10, Line 1539: Reference dbFound,
Reference dbMatched) {
nit: indentation
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1548
PS10, Line 1548: // Make sure we are removing the same instance of Database by
: // checking the CREATION_TIME of the msDb with that of
Catalog's DB and
: // remove the DB from Catalog only if the CREATION_TIME
is equal.
Simplify to ..Remove the db only if the ctime matches with msDB... ?
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1552
PS10, Line 1552: Db removedDb = super.removeDb(dbName);
: if (removedDb != null) {
: updateDeleteLog(removedDb);
: dbMatched.setRef(true);
: }
use this.removeDb() which does all of this for you.
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@874
PS10, Line 874: CatalogServiceCatalog catalog, Metrics metrics,
NotificationEvent event)
nit: 4 space indentation
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@885
PS10, Line 885: throw new MetastoreNotificationException(debugString(
nit: 4 space indentation
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@886
PS10, Line 886: "Database object is null in the event.
Is this the only issue that can cause an "Exception"? Either make it generic
and say something like "Error parsing the event info..." or handle the right
exception,
try {
} catch (DbNullCausingException foo) {
log(Db is null);
} catch (Exception e) {
log(error parsing...)
}
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@909
PS10, Line 909: infoLog("Removed Database {} ", dbName_);
nit: 2space indentation
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@912
PS10, Line 912: "did not exist in catalog.", dbName_);
nit: 4 space indentation
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@915
PS10, Line 915: + "the creation time of the Database did not
match", dbName_));
nit: 4 space indentation
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File
fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@275
PS10, Line 275: Create_DB
nit: CREATE_DB
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@283
PS10, Line 283:
nit: remove unnecessary spaces
http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@296
PS10, Line 296: public void testDropDatabaseCreationTime()
indentation off in multiple places.
--
To view, visit http://gerrit.cloudera.org:8080/12938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c
Gerrit-Change-Number: 12938
Gerrit-PatchSet: 10
Gerrit-Owner: Bharath Krishna
Gerrit-Reviewer: Anurag Mantripragada
Gerrit-Reviewer: Bharath Krishna
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. Patch Set 10: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 10 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Krishna Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Fri, 12 Apr 2019 06:06:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2745/ : 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/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 10 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Krishna Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Fri, 12 Apr 2019 00:40:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2744/ : 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/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Krishna Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Fri, 12 Apr 2019 00:25:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Bharath Krishna has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. Process the drop database event only if the CREATION_TIME of the catalog's database object is lesser than or equal to that of the database object present in the notification event. If the CREATION_TIME in the notification event object is lesser than the catalog's DB object, it means that the Database object present in the catalog is the latest and we can skip the event instead. Testing : - Added unit tests in MetastoreEventsProcessorTest. - Enabled testCreateDropCreateDatabaseFromImpala as we now have CREATION_TIME in the notification events. Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java 3 files changed, 137 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/12938/10 -- To view, visit http://gerrit.cloudera.org:8080/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 10 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Krishna Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Bharath Krishna has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. Process the drop database event only if the CREATION_TIME of the catalog's database object is lesser than or equal to that of the database object present in the notification event. If the CREATION_TIME in the notification event object is lesser than the catalog's DB object, it means that the Database object present in the catalog is the latest and we can skip the event instead. Testing : - Added unit tests in MetastoreEventsProcessorTest. - Enabled testCreateDropCreateDatabaseFromImpala as we now have CREATION_TIME in the notification events. Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java 3 files changed, 136 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/12938/9 -- To view, visit http://gerrit.cloudera.org:8080/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Krishna Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Vihang Karajgaonkar has posted comments on this change. (
http://gerrit.cloudera.org:8080/12938 )
Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid
incorrect deletion of Database objects.
..
Patch Set 8:
(5 comments)
Patch mostly looks good to me. I have one suggestion related to metrics below.
http://gerrit.cloudera.org:8080/#/c/12938/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/12938/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@911
PS7, Line 911: bugLog("Database {}
change to infoLog since this is not a warning and cause of concern.
http://gerrit.cloudera.org:8080/#/c/12938/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/12938/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@208
PS8, Line 208: EVENTS_FILTERED_METRIC
see my comment regarding have a new metric which represent almost the same
thing as events_skipped below.
http://gerrit.cloudera.org:8080/#/c/12938/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@916
PS8, Line 916: EVENTS_FILTERED_METRIC
its not super clear on the difference between events_filtered and
events_skipped. I think we should either merge them together or for now, we
should use events_skipped here.
http://gerrit.cloudera.org:8080/#/c/12938/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/12938/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@202
PS8, Line 202: EVENTS_FILTERED_METRIC
Its confusing to have a events_filtered and events_skipped (not to mention
self_events which are also skipped). Perhaps we should merge them into one. So
we may get rid of this metric and use EVENTS_SKIPPED_METRIC instead? If you do
that also update the comment above EVENTS_SKIPPED_METRIC
http://gerrit.cloudera.org:8080/#/c/12938/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File
fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/12938/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@300
PS8, Line 300: dropDatabaseCascadeFromImpala(TEST_DB_NAME);
: // Create database again with same name
: createDatabaseFromImpala(TEST_DB_NAME, "Test DB for
CREATION_TIME");
: eventsProcessor_.processEvents();
this could be flaky if the drop and create happened in the same second. Do you
want to add a sleep(2sec) here and mention that this is a limitation.
--
To view, visit http://gerrit.cloudera.org:8080/12938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c
Gerrit-Change-Number: 12938
Gerrit-PatchSet: 8
Gerrit-Owner: Bharath Krishna
Gerrit-Reviewer: Anurag Mantripragada
Gerrit-Reviewer: Bharath Krishna
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Thu, 11 Apr 2019 19:43:14 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2720/ : 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/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 8 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Krishna Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 10 Apr 2019 07:43:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Bharath Krishna has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. Process the drop database event only if the CREATION_TIME of the catalog's database object is lesser than or equal to that of the database object present in the notification event. If the CREATION_TIME in the notification event object is lesser than the catalog's DB object, it means that the Database object present in the catalog is the latest and we can skip the event instead. Testing : - Added unit tests in MetastoreEventsProcessorTest. - Enabled testCreateDropCreateDatabaseFromImpala as we now have CREATION_TIME in the notification events. Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java 4 files changed, 129 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/12938/8 -- To view, visit http://gerrit.cloudera.org:8080/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 8 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Krishna Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2718/ : 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/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 7 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Krishna Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 10 Apr 2019 01:47:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Bharath Krishna has posted comments on this change. (
http://gerrit.cloudera.org:8080/12938 )
Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid
incorrect deletion of Database objects.
..
Patch Set 7:
(14 comments)
Thanks for the review Vihang. Updated patch with comments resolved.
http://gerrit.cloudera.org:8080/#/c/12938/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12938/4//COMMIT_MSG@8
PS4, Line 8: incorrect deletion of Database
> I think this is not accurate. creation_time is not used for invalidating bu
Done
http://gerrit.cloudera.org:8080/#/c/12938/4//COMMIT_MSG@15
PS4, Line 15: we can skip the event
: instead.
> suggest you to change it to ".. and we can skip the event instead" which is
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/be/src/util/event-metrics.h
File be/src/util/event-metrics.h:
http://gerrit.cloudera.org:8080/#/c/12938/4/be/src/util/event-metrics.h@40
PS4, Line 40: static DoubleGauge* EVENTS_FETCH_DURATION_MEAN;
> Is there any specific value in exposing this metric on the Catalog metrics
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@953
PS4, Line 953: public List getTableProperties(
> like suggested in my other comment related to race conditions , this API in
Removed this method
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@208
PS4, Line 208:
metrics_.getCounter(MetastoreEventsProcessor.EVENTS_FILTERED_METRIC)
> Just having this line is sufficient to expose the metric in /events page. Y
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@898
PS4, Line 898: vent's DB object, it means that the Database object present
> The process method does not invalidate so this description needs to be upda
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@899
PS4, Line 899: * in the catalog is a later version and we can skip the
event.
> line too long (100 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@903
PS4, Line 903: verride
: public void process() {
: Reference dbFound = new Reference<>();
: Reference dbMatched = new Reference<>();
: Db removedDb =
catalog_.removeDbIfExists(droppedDatabase_, dbFound, dbMatched);
: if (removedDb != null) {
: infoLog("Removed Database {} ", dbName_);
: } else if (!dbMatched.getRef()) {
: LOG.warn(debugString("Database %s was not removed
from catalog since "
: + "the creation time of the Database did not
match", dbName_));
:
metrics_.getCounter(MetastoreEventsProcessor.EVENTS_FILTERED_METRIC).inc();
: } else if (!dbFound.getRef()) {
: debugLog("Database {} was not removed since it did
not exist in catalog
> This code has potential race conditions. For example, the createTime of the
Refactored this method similar to DropTable. Now atomically checking
CREATE_TIME and removing DB
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File
fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@272
PS4, Line 272:* DROP_DATABASE uses CREATION_TIME to filter events that try
to drop an
> add javadoc to describe what exactly this test is testing. Specifically, de
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@284
PS4, Line 284:
> add javadoc
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@285
PS4, Line 285: // Check that the database exists in Catalog
> line too long (126 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@293
PS4, Line 293:*/
> line too long (91 > 90)
Done
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/12938 )
Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid
incorrect deletion of Database objects.
..
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12938/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/12938/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@915
PS7, Line 915: debugLog("Database {} was not removed since it did
not exist in catalog.", dbName_);
line too long (96 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/12938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c
Gerrit-Change-Number: 12938
Gerrit-PatchSet: 7
Gerrit-Owner: Bharath Krishna
Gerrit-Reviewer: Anurag Mantripragada
Gerrit-Reviewer: Bharath Krishna
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Wed, 10 Apr 2019 01:02:59 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Bharath Krishna has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. Process the drop database event only if the CREATION_TIME of the catalog's database object is lesser than or equal to that of the database object present in the notification event. If the CREATION_TIME in the notification event object is lesser than the catalog's DB object, it means that the Database object present in the catalog is the latest and we can skip the event instead. Testing : - Added unit tests in MetastoreEventsProcessorTest. - Enabled testCreateDropCreateDatabaseFromImpala as we now have CREATION_TIME in the notification events. Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java 4 files changed, 128 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/12938/7 -- To view, visit http://gerrit.cloudera.org:8080/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 7 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
