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 2019/03/04 23:46:14 UTC

[Impala-ASF-CR] IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart

Vihang Karajgaonkar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12656


Change subject: IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart
......................................................................

IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart

MetastoreEventsProcessorTest#testEventProcessorFetchAfterHMSRestart
causes test flakiness because the event processor instantiated is not
stopped. This causes the test to have 2 instances of EventsProcessor
running concurrently during the test execution. Depending on the timing
of the poll operations all the events are processed twice and they
modify the state of test catalog concurrently causing race conditions
which lead to intermittent test failures.

Testing done:
1. Confirmed that two event processors are running by manually adding
identifiers to each event processing and log them when they receive the
events.
2. Ran the test multiple times locally to confirm that test works
everytime.
3. Running full-tests using jenkins job.

Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/NoOpEventProcessor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
5 files changed, 62 insertions(+), 34 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/12656/3
-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 3
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................

IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

MetastoreEventsProcessorTest#testEventProcessorFetchAfterHMSRestart
causes test flakiness because the event processor instantiated is not
stopped. This causes the test to have 2 instances of EventsProcessor
running concurrently during the test execution. Depending on the timing
of the poll operations all the events are processed twice and they
modify the state of test catalog concurrently causing race conditions
which lead to intermittent test failures.

The patch also fixes a race condition in testEventSyncFlagTurnedOnErrorCase
where the test assumes and all the tables will be available in catalog
immediately after reset() completes. This assumption is not true and
hence removed the flaky assertNull check in there.

Also, changes the MetastoreEventsProcessorTest to make it easier to
debug through logs. Each individual test case uses a different table
name so that logs pertaining to a test case can be easily grepped.

Testing done:
1. Confirmed that two event processors are running by manually adding
identifiers to each event processing and log them when they receive the
events before this patch. After patch same test does not reveal multiple
event processors running.
2. Ran the test multiple times locally to confirm that test works
everytime.
3. Applied patch for IMPALA-8273 along with this patch and ran
full-tests using jenkins job and see no failure

Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Reviewed-on: http://gerrit.cloudera.org:8080/12656
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/NoOpEventProcessor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
5 files changed, 223 insertions(+), 157 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 13
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................


Patch Set 12: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 12
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 05:23:13 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................

IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

MetastoreEventsProcessorTest#testEventProcessorFetchAfterHMSRestart
causes test flakiness because the event processor instantiated is not
stopped. This causes the test to have 2 instances of EventsProcessor
running concurrently during the test execution. Depending on the timing
of the poll operations all the events are processed twice and they
modify the state of test catalog concurrently causing race conditions
which lead to intermittent test failures.

The patch also fixes a race condition in testEventSyncFlagTurnedOnErrorCase
where the test assumes and all the tables will be available in catalog
immediately after reset() completes. This assumption is not true and
hence removed the flaky assertNull check in there.

Also, changes the MetastoreEventsProcessorTest to make it easier to
debug through logs. Each individual test case uses a different table
name so that logs pertaining to a test case can be easily grepped.

Testing done:
1. Confirmed that two event processors are running by manually adding
identifiers to each event processing and log them when they receive the
events before this patch. After patch same test does not reveal multiple
event processors running.
2. Ran the test multiple times locally to confirm that test works
everytime.
3. Applied patch for IMPALA-8273 along with this patch and ran
full-tests using jenkins job and see no failure

Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/NoOpEventProcessor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
5 files changed, 223 insertions(+), 157 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/12656/10
-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 10
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart
......................................................................


Patch Set 6:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3871/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 6
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 00:23:11 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................

IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

MetastoreEventsProcessorTest#testEventProcessorFetchAfterHMSRestart
causes test flakiness because the event processor instantiated is not
stopped. This causes the test to have 2 instances of EventsProcessor
running concurrently during the test execution. Depending on the timing
of the poll operations all the events are processed twice and they
modify the state of test catalog concurrently causing race conditions
which lead to intermittent test failures.

The patch also fixes a race condition in testEventSyncFlagTurnedOnErrorCase
where the test assumes and all the tables will be available in catalog
immediately after reset() completes. This assumption is not true and
hence removed the flaky assertNull check in there.

Also, changes the MetastoreEventsProcessorTest to make it easier to
debug through logs. Each individual test case uses a different table
name so that logs pertaining to a test case can be easily grepped.

Testing done:
1. Confirmed that two event processors are running by manually adding
identifiers to each event processing and log them when they receive the
events before this patch. After patch same test does not reveal multiple
event processors running.
2. Ran the test multiple times locally to confirm that test works
everytime.
3. Applied patch for IMPALA-8273 along with this patch and ran
full-tests using jenkins job and see no failure

Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/NoOpEventProcessor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
5 files changed, 223 insertions(+), 157 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/12656/11
-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 11
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................

IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

MetastoreEventsProcessorTest#testEventProcessorFetchAfterHMSRestart
causes test flakiness because the event processor instantiated is not
stopped. This causes the test to have 2 instances of EventsProcessor
running concurrently during the test execution. Depending on the timing
of the poll operations all the events are processed twice and they
modify the state of test catalog concurrently causing race conditions
which lead to intermittent test failures.

The patch also fixes a race condition in testEventSyncFlagTurnedOnErrorCase
where the test assumes and all the tables will be available in catalog
immediately after reset() completes. This assumption is not true and
hence removed the flaky assertNull check in there.

Also, changes the MetastoreEventsProcessorTest to make it easier to
debug through logs. Each individual test case uses a different table so
that logs pertaining to a test case can be easily grepped.

Testing done:
1. Confirmed that two event processors are running by manually adding
identifiers to each event processing and log them when they receive the
events before this patch. After patch same test does not reveal multiple
event processors running.
2. Ran the test multiple times locally to confirm that test works
everytime.
3. Running full-tests using jenkins job.

Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/NoOpEventProcessor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
5 files changed, 173 insertions(+), 140 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/12656/7
-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................


Patch Set 12:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3881/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 12
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 01:04:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart
......................................................................

IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart

MetastoreEventsProcessorTest#testEventProcessorFetchAfterHMSRestart
causes test flakiness because the event processor instantiated is not
stopped. This causes the test to have 2 instances of EventsProcessor
running concurrently during the test execution. Depending on the timing
of the poll operations all the events are processed twice and they
modify the state of test catalog concurrently causing race conditions
which lead to intermittent test failures.

Also, changes the MetastoreEventsProcessorTest to make it easier to
debug through logs. Each individual test case uses a different table so
that logs pertaining to a test case can be easily grepped.

Testing done:
1. Confirmed that two event processors are running by manually adding
identifiers to each event processing and log them when they receive the
events.
2. Ran the test multiple times locally to confirm that test works
everytime.
3. Running full-tests using jenkins job.

Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/NoOpEventProcessor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
5 files changed, 174 insertions(+), 140 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/12656/5
-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2345/ : 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/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 00:50:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................


Patch Set 9:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/12656/7/fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java:

http://gerrit.cloudera.org:8080/#/c/12656/7/fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java@53
PS7, Line 53: Cannot be restarted again.
> I think the status of in-flight events should be implementation specific. I
Agreed.


http://gerrit.cloudera.org:8080/#/c/12656/9/fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java:

http://gerrit.cloudera.org:8080/#/c/12656/9/fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java@41
PS9, Line 41: ng
May be also add that "use start(eventId) to resume it..."?


http://gerrit.cloudera.org:8080/#/c/12656/7/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/12656/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@322
PS7, Line 322:     Preconditions.checkNotNull(scheduler_);
> changed the method to synchronized, although I don't think there was a race
ok didn't realize updateStatus was synchronized.


http://gerrit.cloudera.org:8080/#/c/12656/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@323
PS7, Line 323:     Preconditions.checkState(eventProcessorStatus_ != EventProcessorStatus.STOPPED,
> In general, I have not seen many code-bases which do shutdown and awaitTerm
Agree.


http://gerrit.cloudera.org:8080/#/c/12656/9/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/12656/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@173
PS9, Line 173: is a catalog 
?


http://gerrit.cloudera.org:8080/#/c/12656/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@316
PS9, Line 316: n case there are events
             :    * which is being processed currently for a batch, the event is processed. But the
             :    * rest of the events from the batch will not ignored and task will be terminated.
Actually I'm not sure I understand this. The granularity of task here seems to be processEvents(). So, we seem to be waiting for 10s or the current in-flight events to be finished (whichever comes first). Not sure what you mean by "rest of the events" from the batch. Probably needs some re-phrasing.


http://gerrit.cloudera.org:8080/#/c/12656/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@338
PS9, Line 338: 10
Make this a static final int?


http://gerrit.cloudera.org:8080/#/c/12656/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@340
PS9, Line 340: %d
?



-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 9
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 00:25:12 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................


Patch Set 9:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2361/ : 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/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 9
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 23:47:21 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................


Patch Set 7:

> Uploaded patch set 7.

This patch set fixes the testEventSyncFlagTurnedOnErrorCase flakiness due to a race condition. The commit msg and description is updated.


-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 01:47:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................


Patch Set 7:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/12656/7/fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java:

http://gerrit.cloudera.org:8080/#/c/12656/7/fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java@53
PS7, Line 53: Cannot be restarted again.
Mention the status of inflight events?


http://gerrit.cloudera.org:8080/#/c/12656/7/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/12656/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@178
PS7, Line 178:     STOPPED // event processor is not configured
STOPPED sounds like something was running and it was stopped for whatever reason. Should be distinguish between STOPPED and NOT_CONFIGURED?


http://gerrit.cloudera.org:8080/#/c/12656/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@281
PS7, Line 281: stopped
update


http://gerrit.cloudera.org:8080/#/c/12656/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@322
PS7, Line 322:     updateStatus(EventProcessorStatus.STOPPED);
this is probably racy since shutdown() isn't synchronized.


http://gerrit.cloudera.org:8080/#/c/12656/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@323
PS7, Line 323:     scheduler_.shutdownNow();
I think shutdownNow() abruptly terminates inflight threads. Should we instead shutdown and awaitTermination()? Probably doesn't matter much in a real system, but wanted to point out.


http://gerrit.cloudera.org:8080/#/c/12656/7/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/12656/7/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@135
PS7, Line 135:       eventsProcessor_.shutdown();
do it in finally {}?



-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 06:16:35 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2349/ : 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/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 02:18:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................


Patch Set 10:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2366/ : 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/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 10
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 01:28:52 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................

IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

MetastoreEventsProcessorTest#testEventProcessorFetchAfterHMSRestart
causes test flakiness because the event processor instantiated is not
stopped. This causes the test to have 2 instances of EventsProcessor
running concurrently during the test execution. Depending on the timing
of the poll operations all the events are processed twice and they
modify the state of test catalog concurrently causing race conditions
which lead to intermittent test failures.

The patch also fixes a race condition in testEventSyncFlagTurnedOnErrorCase
where the test assumes and all the tables will be available in catalog
immediately after reset() completes. This assumption is not true and
hence removed the flaky assertNull check in there.

Also, changes the MetastoreEventsProcessorTest to make it easier to
debug through logs. Each individual test case uses a different table
name so that logs pertaining to a test case can be easily grepped.

Testing done:
1. Confirmed that two event processors are running by manually adding
identifiers to each event processing and log them when they receive the
events before this patch. After patch same test does not reveal multiple
event processors running.
2. Ran the test multiple times locally to confirm that test works
everytime.
3. Applied patch for IMPALA-8273 along with this patch and ran
full-tests using jenkins job and see no failure

Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/catalog/events/NoOpEventProcessor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
5 files changed, 217 insertions(+), 157 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/12656/9
-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 9
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2344/ : 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/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 3
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 00:14:43 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................


Patch Set 12: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 12
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 01:04:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................


Patch Set 11:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2367/ : 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/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 11
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 01:47:04 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................


Patch Set 11: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 11
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 01:04:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix MetastoreEventsProcessorTest flakiness
......................................................................


Patch Set 6: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 6
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 04:39:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12656 )

Change subject: IMPALA-8278 : Fix testEventProcessorFetchAfterHMSRestart
......................................................................


Patch Set 4:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3870/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/12656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia12b1d048d8c8c59fa4c587bf65772ff194d314d
Gerrit-Change-Number: 12656
Gerrit-PatchSet: 4
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Mar 2019 23:49:15 +0000
Gerrit-HasComments: No