You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org> on 2023/11/09 16:39:49 UTC
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Wenzhe Zhou has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20689
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
Test test_statestored_auto_failover_with_disabling_network failed
occasionally due to delay of HA Handshake or HA heartbeat RPCs between
two statestore instances. Sometimes the active statestore took a few
minutes to response the handshake requests from standby statestore.
This patch fixs the issue by not holding mutex ha_lock_ when sending
HA handshake and HA heartbeat. Redundant HA heartbeats are handled
on receiver side. Redundant HA handshakes are harmless.
Testing:
- Repeatedly ran test_statestored_auto_failover_with_disabling_network
on Jenkins for hundreds of times without failure.
- Repeatedly ran test_statestored_auto_failover_with_disabling_network
on local machine for thousand times without failure.
- Repeatedly ran all tests in test_statestored_ha.py for over 12
hours on Jenkins without failure.
- Passed core tests.
Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
---
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/custom_cluster/test_statestored_ha.py
4 files changed, 71 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/20689/2
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
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/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
Test test_statestored_auto_failover_with_disabling_network failed
occasionally due to delay of HA Handshake or HA heartbeat RPCs between
two statestore instances. Sometimes the active statestore took a few
minutes to respond to the handshake requests from standby statestore.
This patch fixes the issue by not holding mutex ha_lock_ when sending
HA handshake and HA heartbeat. Redundant HA heartbeats are handled
on receiver side. Redundant HA handshakes are harmless.
Testing:
- Repeatedly ran test_statestored_auto_failover_with_disabling_network
on Jenkins for hundreds of times without failure.
- Repeatedly ran test_statestored_auto_failover_with_disabling_network
on local machine for thousand times without failure.
- Repeatedly ran all tests in test_statestored_ha.py for over 12 hours
on Jenkins without failure.
- Passed core tests.
Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Reviewed-on: http://gerrit.cloudera.org:8080/20689
Reviewed-by: Riza Suminto <ri...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/custom_cluster/test_statestored_ha.py
4 files changed, 87 insertions(+), 22 deletions(-)
Approvals:
Riza Suminto: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 6
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
Test test_statestored_auto_failover_with_disabling_network failed
occasionally due to delay of HA Handshake or HA heartbeat RPCs between
two statestore instances. Sometimes the active statestore took a few
minutes to responnd to the handshake requests from standby statestore.
This patch fixes the issue by not holding mutex ha_lock_ when sending
HA handshake and HA heartbeat. Redundant HA heartbeats are handled
on receiver side. Redundant HA handshakes are harmless.
Testing:
- Repeatedly ran test_statestored_auto_failover_with_disabling_network
on Jenkins for hundreds of times without failure.
- Repeatedly ran test_statestored_auto_failover_with_disabling_network
on local machine for thousand times without failure.
- Repeatedly ran all tests in test_statestored_ha.py for over 12
hours on Jenkins without failure.
- Passed core tests.
Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
---
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/custom_cluster/test_statestored_ha.py
4 files changed, 72 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/20689/3
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9914/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 20:32:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Nov 2023 01:04:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14403/ : 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/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 20:57:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20689/4/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/20689/4/be/src/statestore/statestore.cc@1976
PS4, Line 1976: LOG(WARNING) << "Enter HA recovery mode.";
> Good suggestion. Will change in the future. Added TODO here
A couple places are accumulating several values that need to be set together. Pulling those out into helper functions would help avoid mistakes. Something else useful to think about when refactoring.
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 21:20:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 5:
Thanks Michael and Riza for your reviews.
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 20:31:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
Test test_statestored_auto_failover_with_disabling_network failed
occasionally due to delay of HA Handshake or HA heartbeat RPCs between
two statestore instances. Sometimes the active statestore took a few
minutes to respond to the handshake requests from standby statestore.
This patch fixes the issue by not holding mutex ha_lock_ when sending
HA handshake and HA heartbeat. Redundant HA heartbeats are handled
on receiver side. Redundant HA handshakes are harmless.
Testing:
- Repeatedly ran test_statestored_auto_failover_with_disabling_network
on Jenkins for hundreds of times without failure.
- Repeatedly ran test_statestored_auto_failover_with_disabling_network
on local machine for thousand times without failure.
- Repeatedly ran all tests in test_statestored_ha.py for over 12 hours
on Jenkins without failure.
- Passed core tests.
Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
---
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/custom_cluster/test_statestored_ha.py
4 files changed, 87 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/20689/5
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 2: Code-Review+1
(4 comments)
I think I understand what's going on there, just had a couple typos and comments that would help clarify it.
http://gerrit.cloudera.org:8080/#/c/20689/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20689/2//COMMIT_MSG@12
PS2, Line 12: minutes to response the handshake requests from standby statestore.
nit: "to respond to the"
http://gerrit.cloudera.org:8080/#/c/20689/2//COMMIT_MSG@14
PS2, Line 14: This patch fixs the issue by not holding mutex ha_lock_ when sending
nit: "fixes"
http://gerrit.cloudera.org:8080/#/c/20689/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/20689/2/be/src/statestore/statestore.cc@720
PS2, Line 720: num_received_heartbeat_in_active_ = 0;
We reset this whenever `is_active_` is set to true, right? Might be worth a comment for anyone updating this file in the future.
http://gerrit.cloudera.org:8080/#/c/20689/2/be/src/statestore/statestore.cc@2128
PS2, Line 2128: lock_guard<mutex> l(ha_lock_);
Why does this require a lock? Because it's accessing peer_statestore_id_?
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 18:06:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20689/4/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/20689/4/be/src/statestore/statestore.cc@1976
PS4, Line 1976: LOG(WARNING) << "Enter HA recovery mode.";
> A couple places are accumulating several values that need to be set togethe
Thanks for suggestion. I will add more help functions when refactoring.
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 21:55:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20689/4/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/20689/4/be/src/statestore/statestore.cc@211
PS4, Line 211: // If statestore instance in active state receives more than 10 heartbeats from its peer,
: // enter recovery mode to re-negotiate role with its peer.
> By default, how long is it in terms of second? 10 second?
Updated comments
http://gerrit.cloudera.org:8080/#/c/20689/4/be/src/statestore/statestore.cc@1976
PS4, Line 1976: LOG(WARNING) << "Enter HA recovery mode.";
> nit: Not asking in this patch, but I think it is worth to break this functi
Good suggestion. Will change in the future. Added TODO here
http://gerrit.cloudera.org:8080/#/c/20689/4/be/src/statestore/statestore.cc@2006
PS4, Line 2006: in_ha_recovery_mode_metric_->SetValue(in_recovery_mode_);
: peer_statestore_id_ = response.dst_statestore_id;
> nit: Is it possible to also print how long this statestored stays in recove
Good suggestion. Added elapsed time in the log message.
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 20:29:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 4: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/20689/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20689/2//COMMIT_MSG@12
PS2, Line 12: minutes to respond to the handshake requests from standby statestore.
> fixed
Done
http://gerrit.cloudera.org:8080/#/c/20689/2//COMMIT_MSG@14
PS2, Line 14: This patch fixes the issue by not holding mutex ha_lock_ when sending
> fixed
Done
http://gerrit.cloudera.org:8080/#/c/20689/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/20689/2/be/src/statestore/statestore.cc@720
PS2, Line 720: num_received_heartbeat_in_active_ = 0;
> added a comment in statestore.h
Done
http://gerrit.cloudera.org:8080/#/c/20689/2/be/src/statestore/statestore.cc@2128
PS2, Line 2128: lock_guard<mutex> l(ha_lock_);
> Yes, that variable could be changed by other threads.
Ack
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 18:29:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14397/ : 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/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 18:54:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20689/4/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/20689/4/be/src/statestore/statestore.cc@211
PS4, Line 211: // If statestore instance in active state receives more than 10 heartbeats from its peer,
: // enter recovery mode to re-negotiate role with its peer.
By default, how long is it in terms of second? 10 second?
Can you mention the related flag about heartbeat period here?
http://gerrit.cloudera.org:8080/#/c/20689/4/be/src/statestore/statestore.cc@1976
PS4, Line 1976: [[noreturn]] void Statestore::MonitorStatestoredHaHeartbeat() {
nit: Not asking in this patch, but I think it is worth to break this function to 3 function for each branch (IsInRecoveryMode(), IsActive(), and is in standby state) in the future.
http://gerrit.cloudera.org:8080/#/c/20689/4/be/src/statestore/statestore.cc@2006
PS4, Line 2006: LOG(INFO) << "Receive Statestore HA handshake response, exit HA recovery mode. "
: << "Set the statestored as " << (is_active_ ? "active" : "standby");
nit: Is it possible to also print how long this statestored stays in recovery mode? Or is there any metrics for it?
I think it will be helpful to debug future issues.
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 19:21:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14396/ : 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/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 17:06:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/20689/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20689/2//COMMIT_MSG@12
PS2, Line 12: minutes to respond to the handshake requests from standby statestore.
> You added an extra "n".
fixed
http://gerrit.cloudera.org:8080/#/c/20689/2//COMMIT_MSG@12
PS2, Line 12: minutes to respond to the handshake requests from standby statestore.
> nit: "to respond to the"
fixed
http://gerrit.cloudera.org:8080/#/c/20689/2//COMMIT_MSG@14
PS2, Line 14: This patch fixes the issue by not holding mutex ha_lock_ when sending
> nit: "fixes"
fixed
http://gerrit.cloudera.org:8080/#/c/20689/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/20689/2/be/src/statestore/statestore.cc@720
PS2, Line 720: num_received_heartbeat_in_active_ = 0;
> We reset this whenever `is_active_` is set to true, right? Might be worth a
added a comment in statestore.h
http://gerrit.cloudera.org:8080/#/c/20689/2/be/src/statestore/statestore.cc@2128
PS2, Line 2128: lock_guard<mutex> l(ha_lock_);
> Why does this require a lock? Because it's accessing peer_statestore_id_?
Yes, that variable could be changed by other threads.
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 18:27:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
Test test_statestored_auto_failover_with_disabling_network failed
occasionally due to delay of HA Handshake or HA heartbeat RPCs between
two statestore instances. Sometimes the active statestore took a few
minutes to respond to the handshake requests from standby statestore.
This patch fixes the issue by not holding mutex ha_lock_ when sending
HA handshake and HA heartbeat. Redundant HA heartbeats are handled
on receiver side. Redundant HA handshakes are harmless.
Testing:
- Repeatedly ran test_statestored_auto_failover_with_disabling_network
on Jenkins for hundreds of times without failure.
- Repeatedly ran test_statestored_auto_failover_with_disabling_network
on local machine for thousand times without failure.
- Repeatedly ran all tests in test_statestored_ha.py for over 12
hours on Jenkins without failure.
- Passed core tests.
Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
---
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/custom_cluster/test_statestored_ha.py
4 files changed, 72 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/20689/4
--
To view, visit http://gerrit.cloudera.org:8080/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-12550: Fix flaky test test statestored auto failover with disabling network
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20689 )
Change subject: IMPALA-12550: Fix flaky test test_statestored_auto_failover_with_disabling_network
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14398/ : 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/20689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515bbaaddfb4bf9bd2a39414cd6e3e4590dfbfb1
Gerrit-Change-Number: 20689
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 18:54:22 +0000
Gerrit-HasComments: No