You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org> on 2021/09/07 20:36:41 UTC
[Impala-ASF-CR] IMPALA-10907: Refactor MetastoreEvents class
Vihang Karajgaonkar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17833
Change subject: IMPALA-10907: Refactor MetastoreEvents class
......................................................................
IMPALA-10907: Refactor MetastoreEvents class
This is a refactor only patch. Before the patch
the MetastoreEvents.java was a single file
which included bunch of inner classes for each metastore
event type. This file had become quite large and this
patch breaks down this file into bunch of smaller
classes to improve code readability.
Testing:
No new tests were needed since this is a code
refactor.
Change-Id: I71edd17e6705f81cd402f9aecc86ef3ffff50a76
---
M fe/src/main/java/org/apache/impala/catalog/TableLoader.java
A fe/src/main/java/org/apache/impala/catalog/events/AddPartitionEvent.java
A fe/src/main/java/org/apache/impala/catalog/events/AlterDatabaseEvent.java
A fe/src/main/java/org/apache/impala/catalog/events/AlterPartitionEvent.java
A fe/src/main/java/org/apache/impala/catalog/events/AlterTableEvent.java
A fe/src/main/java/org/apache/impala/catalog/events/CreateDatabaseEvent.java
A fe/src/main/java/org/apache/impala/catalog/events/CreateTableEvent.java
M fe/src/main/java/org/apache/impala/catalog/events/DeleteEventLog.java
A fe/src/main/java/org/apache/impala/catalog/events/DropDatabaseEvent.java
A fe/src/main/java/org/apache/impala/catalog/events/DropPartitionEvent.java
A fe/src/main/java/org/apache/impala/catalog/events/DropTableEvent.java
M fe/src/main/java/org/apache/impala/catalog/events/EventFactory.java
M fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java
A fe/src/main/java/org/apache/impala/catalog/events/IgnoredEvent.java
A fe/src/main/java/org/apache/impala/catalog/events/InsertEvent.java
A fe/src/main/java/org/apache/impala/catalog/events/MetastoreDatabaseEvent.java
A fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvent.java
A fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventFactory.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventProcessorConfig.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/main/java/org/apache/impala/catalog/events/MetastoreNotificationException.java
A fe/src/main/java/org/apache/impala/catalog/events/MetastoreTableEvent.java
M fe/src/main/java/org/apache/impala/catalog/events/NoOpEventProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/SelfEventContext.java
M fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
28 files changed, 1,705 insertions(+), 1,544 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/17833/1
--
To view, visit http://gerrit.cloudera.org:8080/17833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I71edd17e6705f81cd402f9aecc86ef3ffff50a76
Gerrit-Change-Number: 17833
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-10907: Refactor MetastoreEvents class
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17833 )
Change subject: IMPALA-10907: Refactor MetastoreEvents class
......................................................................
Patch Set 1:
(2 comments)
Thanks for the refactor! I also found MatastoreEvent.java too long.
http://gerrit.cloudera.org:8080/#/c/17833/1/fe/src/main/java/org/apache/impala/catalog/events/AddPartitionEvent.java
File fe/src/main/java/org/apache/impala/catalog/events/AddPartitionEvent.java:
http://gerrit.cloudera.org:8080/#/c/17833/1/fe/src/main/java/org/apache/impala/catalog/events/AddPartitionEvent.java@1
PS1, Line 1: package org.apache.impala.catalog.events;
Licenses are missing (which have led to the broken build)
http://gerrit.cloudera.org:8080/#/c/17833/1/fe/src/main/java/org/apache/impala/catalog/events/AddPartitionEvent.java@35
PS1, Line 35: Preconditions.checkState(eventType_.equals(MetastoreEventType.ADD_PARTITION));
: if (event.getMessage() == null) {
: throw new IllegalStateException(debugString("Event message is null"));
: }
: try {
: AddPartitionMessage addPartitionMessage_ =
: MetastoreEventsProcessor.getMessageDeserializer()
: .getAddPartitionMessage(event.getMessage());
optional idea for improvement:
I think that the amount of code could be reduced by moving this logic (and its friends in other classes) to the factory. The constructors could take and AddPartitionMessage/similar argument.
The Exception-> MetastoreNotificationException logic could be probably also moved to the factory.
--
To view, visit http://gerrit.cloudera.org:8080/17833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71edd17e6705f81cd402f9aecc86ef3ffff50a76
Gerrit-Change-Number: 17833
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Sep 2021 16:50:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10907: Refactor MetastoreEvents class
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17833 )
Change subject: IMPALA-10907: Refactor MetastoreEvents class
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/9436/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71edd17e6705f81cd402f9aecc86ef3ffff50a76
Gerrit-Change-Number: 17833
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Sep 2021 20:52:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10907: Refactor MetastoreEvents class
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17833 )
Change subject: IMPALA-10907: Refactor MetastoreEvents class
......................................................................
Patch Set 1:
There are some more critical fixes and improvements in review for events processor. Since this patch will disrupt them, I am waiting for them to be merged and then I will update this patch.
--
To view, visit http://gerrit.cloudera.org:8080/17833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71edd17e6705f81cd402f9aecc86ef3ffff50a76
Gerrit-Change-Number: 17833
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Oct 2021 18:43:59 +0000
Gerrit-HasComments: No