You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Lars Volker (Code Review)" <ge...@cloudera.org> on 2019/05/01 18:17:18 UTC
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Lars Volker has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13207
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaced the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change, it adds maps to store multiple
executor groups.
Testing: This change does not introduce new functionality and the new
class is covered by the existing scheduler unit test and the end to end
tests.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/common/logging.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M common/thrift/StatestoreService.thrift
24 files changed, 1,124 insertions(+), 915 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/2
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 5:
(20 comments)
Thanks for the review. Please see my inline comments and PS6.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/runtime/exec-env.cc@454
PS5, Line 454: // Register the ImpalaServer with the cluster membership manager
: cluster_membership_mgr_->SetLocalBeDescFn([server]() {
: return server->GetLocalBackendDescriptor();
: });
: cluster_membership_mgr_->SetUpdateLocalServerFn(
: [server](const ClusterMembershipMgr::BackendAddressSet& current_backends) {
: server->CancelQueriesOnFailedBackends(current_backends);
: });
> just thinking out aloud: should we reset the callback functions during tear
These rely only on the server still being alive, not the ExecEnv, so the right thing to do would be to delete them when we unregister the ImpalaServer from the ExecEnv (which we currently don't). I added a DCHECK to make sure that we don't use this method to reset the ImpalaServer in the future. Since the ExecEnv d'tor will destroy the ClusterMembershipManager I think we should be good without resetting its state explicitly.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@54
PS5, Line 54: /// Clients can also register callbacks to receive notifications of changes to the cluster
: /// membership.
> this makes it sound like there is a generic way of registering callbacks li
Done
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@68
PS5, Line 68: pool
> nit: executor pool. Is there a description of "executor pools" anywhere?
Replaced it with executor groups and added a brief description which we can expand in the future.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@102
PS5, Line 102: then
> nit: when
Done
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@112
PS5, Line 112: subscription
> nit: subscription.
Done
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@117
PS5, Line 117: /// Registers a callback to provide the local backend descriptor.
: void SetLocalBeDescFn(BackendDescriptorPtrFn fn);
:
: /// Registers a callback to notify the local ImpalaServer of changes in the cluster
: /// membership. This callback will only be called when backends are deleted from the
: /// membership.
: void SetUpdateLocalServerFn(UpdateLocalServerFn fn);
:
: /// Registers a callback to notify the local Frontend of changes in the cluster
: /// membership.
: void SetUpdateFrontendFn(UpdateFrontendFn fn);
> I know we discussed this offline, but it might be worth documenting in this
Both clients have different needs: The ImpalaServer only needs to learn about deleted backends, whereas the Frontend needs the full list. In the future, the Frontend will also need the executor group sizes (but not the memberships because groups will likely only be useful for remote read scenarios). Additionally, the callbacks have different signatures. Having a generic callback interface would require some sort of filtering of events (added, updated, deleted) and more complexity on the client side without a clear benefit. If the list of client classes expands we can revisit this decision.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@165
PS5, Line 165: in
> nit: extra word
Done
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@167
PS5, Line 167: / occur in 'current_backends'
> nit: outdated comment?
Done
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@184
PS5, Line 184: May be NULL if the set of
: /// backends is fixed.
> maybe mention that this is only true in tests
Done
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc@88
PS5, Line 88: update.is_delta && update.topic_entries.empty()
> just curious, when can we receive an empty delta
It's the way that the statestore pulls for updates from the clients, e.g. every time interval it will send an empty delta and the clients will reply with their updates. It cannot send an empty snapshot because that would delete the whole state, and sending the full membership each time is too much overhead.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc@127
PS5, Line 127: const vector<string>& groups = DEFAULT_EXECUTOR_GROUPS;
: for (const string& group : groups) {
: (*new_executor_groups)[group].RemoveExecutor(be_desc);
: }
> This kinda implies that an executor can be added to multiple groups. I thin
I added a comment and TODO to the class header, let me know if you'd like me to elaborate more.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc@255
PS5, Line 255: auto
> nit: const auto&
Done. The const is redundant but I added it for readability. (See https://gerrit.cloudera.org/#/c/12065/8/be/src/exec/parquet/parquet-common.cc@95)
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc@263
PS5, Line 263: const BackendIdMap::value_type&
> nit: const auto&
Done
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc@297
PS5, Line 297: auto
> nit: const auto&
Done
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/executor-group.h
File be/src/scheduling/executor-group.h:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/executor-group.h@41
PS5, Line 41: The caller must make sure that the
: /// executor group does not get updated while they hold the reference.
> would it make sense to document what is the expected usage pattern for this
I removed this comment and added an explanation to the class comment.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/executor-group.h@49
PS5, Line 49: Note that during tests this can include multiple
: /// executors per ip address.
> any reason we are specifying this here? the class description already impli
There's an ambiguity between this method, GetAllExecutorIps(), and NumExecutors() that is only relevant during tests. I also added a comment to NumExecutors() and to the class itself to point it out.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.h
File be/src/scheduling/scheduler.h:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.h@a334
PS5, Line 334:
> we can add something like this back when we introduce exec groups.
Yeah, we'd probably want to add metrics for total backends and executors not in shutdown. Then the python tests could use those instead of the /backends debug page. Should we file a follow-up Jira (might be a good rampup task) or fix it here?
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.h@145
PS5, Line 145: the global executor
> nit: maybe call this an "executor group"?
Done
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.cc@439
PS5, Line 439: // TODO(lars)
> marking this in case you missed it
Thanks, I think this is actually a good place to keep it (easier to test).
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.cc@654
PS5, Line 654: if (membership_snapshot->local_be_desc.get() == nullptr) {
: return Status("Local backend has not yet started");
: }
> can this happen? as you can only submit queries once the impala server is r
There's a race between the ImpalaServer starting up and the statestore topic update triggering the registration in the local backend descriptor. The upside of it is a looser coupling between the classes. The alternative would be to explicitly call from the ImpalaServer to the ClusterMembershipManager upon startup and add the local backend descriptor, but then we'd have two paths concurrently updating the local snapshot which would require more careful synchronization.
I clarified the error message and added a comment.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 May 2019 20:44:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 23: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 23
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Jun 2019 17:14:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 11:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/3276/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 11
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 May 2019 23:15:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 23:
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4376/
Looks like it hit IMPALA-8610
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 23
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Jun 2019 17:45:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#16).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaces the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change (IMPALA-8484), it adds maps to store
multiple executor groups.
This change also disables the generation of default operators from the
thrift files and instead adds explicit implementations for the ones that
we rely on. This forces us to explicitly specify comparators when
manipulating containers of thrift structs and will help prevent
accidental bugs.
Testing: This change adds a backend unit test for the new cluster
membership manager. The observable behavior of Impala does not change,
and the existing scheduler unit test and end to end tests should make
sure of that.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/common/thread-debug-info-test.cc
M be/src/experiments/hash-ring-util.cc
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/query-state.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr-test.cc
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/cluster-membership-test-util.cc
A be/src/scheduling/cluster-membership-test-util.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/runtime-profile-test.cc
M be/src/util/uid-util-test.cc
M common/thrift/CMakeLists.txt
M common/thrift/Frontend.thrift
M common/thrift/StatestoreService.thrift
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/custom_cluster/test_coordinators.py
48 files changed, 1,920 insertions(+), 1,012 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/16
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 16
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 15:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3443/ : 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/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 15
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 May 2019 18:20:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 19:
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4358/
The build hit a race condition where for a short time during statestore re-connection the connection had already been established, but the last_recovery_duration_metric_ had not yet been updated. To fix this, I added a new metric (see updated commit message) that keeps track of the actual number of reconnects.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 19
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 May 2019 18:05:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 17: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.h@210
PS17, Line 210: /// post-recovery grace period. Not exposed to clients and not protected by any locking.
This is only accessed by UpdateMembership(),right? I.e. "protected" by update_membership_lock_
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.cc@272
PS17, Line 272: if (update_local_server) NotifyLocalServerForDeletedBackend(*new_backend_map);
I guess this means we call NotifyLocalServerForDeletedBackend() in more cases (i.e. no backend was necessarily removed). I think this is ok.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 17
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 May 2019 21:46:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#20).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaces the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also generalizes the fix for IMPALA-7665: updates from the
statestore to the cluster membership topic are only made visible to the
rest of the local server after a post-recovery grace period has elapsed.
As part of this the flag
'failed_backends_query_cancellation_grace_period_ms' is replaced with
'statestore_subscriber_recovery_grace_period_ms'. To tell the initial
startup from post-recovery, a new metric
'statestore-subscriber.num-connection-failures' is exposed by the
daemon, which tracks the total number of connection failures to the
statestore over the lifetime process lifetime.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change (IMPALA-8484), it adds maps to store
multiple executor groups.
This change also disables the generation of default operators from the
thrift files and instead adds explicit implementations for the ones that
we rely on. This forces us to explicitly specify comparators when
manipulating containers of thrift structs and will help prevent
accidental bugs.
Testing: This change adds a backend unit test for the new cluster
membership manager. The observable behavior of Impala does not change,
and the existing scheduler unit test and end to end tests should make
sure of that.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/common/thread-debug-info-test.cc
M be/src/experiments/hash-ring-util.cc
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/query-state.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr-test.cc
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/cluster-membership-test-util.cc
A be/src/scheduling/cluster-membership-test-util.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/runtime-profile-test.cc
M be/src/util/uid-util-test.cc
M common/thrift/CMakeLists.txt
M common/thrift/Frontend.thrift
M common/thrift/StatestoreService.thrift
M common/thrift/metrics.json
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/custom_cluster/test_coordinators.py
M tests/custom_cluster/test_restart_services.py
50 files changed, 1,994 insertions(+), 1,027 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/20
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 20
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/executor-group-test.cc
File be/src/scheduling/executor-group-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/executor-group-test.cc@61
PS11, Line 61: executor_group.RemoveExecutor(MakeBackendDescriptor(2));
> I'm not sure I understand the question. In this particular case it won't be
Crashes of the test binary will cause a test failure. It's actually a pretty common failure mode for our tests because failed assertions cause some teardown logic to be skipped and DCHECKs get tripped as a result of resource leaks, etc.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 11
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 May 2019 21:41:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#3).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaced the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change, it adds maps to store multiple
executor groups.
Testing: This change does not introduce new functionality and the new
class is covered by the existing scheduler unit test and the end to end
tests.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M common/thrift/StatestoreService.thrift
25 files changed, 1,130 insertions(+), 914 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/3
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 7: Code-Review+1
Carrying Bikram's +1
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 7
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 May 2019 20:51:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 14: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr.cc@256
PS11, Line 256: // Setting this flag al
> Technically because it's the right thing to do: This coordinator only provi
I see, thanks for the explanation. I initially thought its only the statestore's responsibility to set this but now reading through the descriptions in the thrift file it seems like this should always be set to 'true' for updates sent by a subscriber as the subscriber always sends updates relative to the received version
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 14
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 May 2019 19:04:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 11:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@120
PS11, Line 120: int num_expected_updates = -1
nit: unused
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@149
PS11, Line 149: but does not remove the
: /// backend from that list after creating its CMM.
so those lists are not exclusive, that is, their members can overlap. Can you mention that briefly because after reading "Backends can be in one of 5 states" my initial thought was that those lists maintain this invariant
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@304
PS11, Line 304: mgr
nit: backend
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@342
PS11, Line 342: // Quiesce them all
how about Quiesce half of it, to exercise the path where backends later get deleted directly. Although i know that would be covered by the next test, it might still make sense to go through it deterministically
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@391
PS11, Line 391: P_SHUTDOWN
nit: maybe call it P_QUIESCE to be consistent
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr.cc@350
PS11, Line 350: : is_quiescing"
nit: maybe print here and below what the value is for group_be.is_quiescing
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/executor-group-test.cc
File be/src/scheduling/executor-group-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/executor-group-test.cc@61
PS11, Line 61: executor_group.RemoveExecutor(MakeBackendDescriptor(2));
will the LOG DFATAL result in an exit here and cause the test to fail?
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 11
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 May 2019 20:56:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#8).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaced the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change, it adds maps to store multiple
executor groups.
Testing: This change does not introduce new functionality and the new
class is covered by the existing scheduler unit test and the end to end
tests.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M common/thrift/StatestoreService.thrift
M tests/custom_cluster/test_coordinators.py
26 files changed, 1,177 insertions(+), 923 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/8
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 8
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 20:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3464/ : 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/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 20
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 May 2019 18:43:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 12:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/3367/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 12
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 May 2019 21:17:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 17:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3450/ : 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/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 17
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 May 2019 21:36:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 14:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3424/ : 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/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 14
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 May 2019 18:13:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 3:
(25 comments)
I did an initial quick pass, focused mainly on the interfaces and overall code structure. I haven't made a concerted effort to think about correctness and/or look for bugs yet but I thought it was worth flushing out the feedback at this point.
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.h@96
PS2, Line 96: /// A callback to notify the local ImpalaServer of changes to the cluster membership.
Should we document that no locks are held during by the calling thread of the callback? That's the trickiest problem I've generally seen with callbacks and it's not an issue here.
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@94
PS2, Line 94: not
nit: no
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@98
PS2, Line 98: VLOG(3) << "Processing statestore update";
I feel like a lot of these messages could actually be VLOG(2) or even VLOG(1), since they're relatively rare and mean *something* happened, like a cluster membership change or this impalad registering itself. Don't need to fix it right now, but worth thinking about if it would help debugging in future.
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@108
PS2, Line 108: // Make a copy. Currently no other code would modify the state but we take the lock as
We're assuming that only one thread executes the below code at a time, right, otherwise they'd race to update the sate. Can we use a utility to verify this - I believe DFAKE_SCOPED_LOCK_THREAD_LOCKED from gutil does this, but I'm just going on Todd's advice.
Ok to defer but would be nice to clarify invariants as part of refactor.
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@113
PS2, Line 113: if (local_be_desc.get() != nullptr) {
Conditional fits on one line?
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@145
PS2, Line 145: VLOG(2) << "Error deserializing membership topic item with key: " << item.key;
This seems like something we should definitely log at INFO or probably even WARNING level, since something went wrong!
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@152
PS2, Line 152: VLOG(1) << "Ignoring subscription request with empty IP address from subscriber: "
Same here
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@201
PS2, Line 201: desccriptor
typo
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@242
PS2, Line 242: LOG(WARNING) << "Failed to serialize Impala backend descriptor for statestore topic:"
It seems like this really should be a LOG(FATAL) or a CHECK since we never would expect this to happen and an Impalad that can't register itself with the SS is useless.
OK to ignore to avoid scope creep of refactor.
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@252
PS2, Line 252: BackendAddressSet current_backend_set;
This is O(n^2) where n in the cluster size, in some sense because we potentially do O(n) work for each of O(n) registrations. I think this is probably OK though right? Because generally the membership changes won't be continual and this is fairly cheap.
I do feel like this is less error prone than trying to only send deltas, so I think I'm in favour.
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@282
PS2, Line 282: const BackendDescriptorPtr& local_be_desc){
nit: whitespace
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@283
PS2, Line 283: if (local_be_desc.get() == nullptr) return false;
I don't personally like the redundant .get() here but it is a matter of taste :)
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@312
PS2, Line 312: << " is shutting down";
return false here and on l308?
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h
File be/src/scheduling/executor-group.h:
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@38
PS3, Line 38: ExecutorGroup(const std::vector<TNetworkAddress>& executors);
Explicit constructors?
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@41
PS3, Line 41: typedef std::list<TBackendDescriptor> ExecutorList;
Any reason this shouldn't be a vector? That's generally a better default. It doesn't look like we need stable iterators into a list, I don't think and I don't see anywhere where the asymptotic complexity is better.
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@43
PS3, Line 43: /// Return the list of executors on a particular host. The caller must make sure that
What's the thread safety story for the returned list?
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@47
PS3, Line 47: void GetAllExecutorIps(std::vector<IpAddr>* ip_addresses) const;
Can you document whether these append to the vectors or replace the contents. And if it replaces the contents, why not just return a new vector?
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@49
PS3, Line 49: void AddExecutor(const TBackendDescriptor& be_desc);
I think these could do with a basic description. There may be some details worth documenting, e.g. what happens if the executor already exists in the group.
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@62
PS3, Line 62: /// lifetime of this ExecutorGroup.
What happens if the executor is removed with RemoveExecutor()?
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@65
PS3, Line 65: const HashRing* GetHashRing() const { return &executor_ip_hash_ring_; }
Maybe worth documenting lifetime/thread safety of this?
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@71
PS3, Line 71: typedef boost::unordered_map<IpAddr, ExecutorList> ExecutorMap;
Maybe a good time to switch these to std::unordered_map?
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.cc
File be/src/scheduling/executor-group.cc:
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.cc@54
PS3, Line 54: ip_addresses->reserve(NumExecutors());
This isn't right if ip_addresses already has entries. Not sure if this happens in practice - if it doesn't though we should just return the vector I think.
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.cc@109
PS3, Line 109: LIKELY
LIKELY annotation probably isn't necessary. Ok to leave.
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/scheduler.h
File be/src/scheduling/scheduler.h:
PS3:
It's nice that the scheduler has become more focused on scheduling and less stateful.
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/service/impala-server.cc@a1744
PS3, Line 1744:
:) Nice to see this file get slightly simpler
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 May 2019 17:26:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13207/12/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/12/be/src/scheduling/cluster-membership-mgr-test.cc@391
PS12, Line 391: int num_expected_alive = backends_.size() - to_delete;
> Done. Note that in DeleteBackend() we also ensure that the backend was remo
Ah right, yeah it doesn't seem worth tackling now unless you see an easy solution (the check seems worthwhile).
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 13
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 May 2019 19:03:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#15).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaces the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change (IMPALA-8484), it adds maps to store
multiple executor groups.
This change also disables the generation of default operators from the
thrift files and instead adds explicit implementations for the ones that
we rely on. This forces us to explicitly specify comparators when
manipulating containers of thrift structs and will help prevent
accidental bugs.
Testing: This change adds a backend unit test for the new cluster
membership manager. The observable behavior of Impala does not change,
and the existing scheduler unit test and end to end tests should make
sure of that.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/common/thread-debug-info-test.cc
M be/src/experiments/hash-ring-util.cc
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/query-state.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr-test.cc
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/cluster-membership-test-util.cc
A be/src/scheduling/cluster-membership-test-util.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/runtime-profile-test.cc
M be/src/util/uid-util-test.cc
M common/thrift/CMakeLists.txt
M common/thrift/Frontend.thrift
M common/thrift/StatestoreService.thrift
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/custom_cluster/test_coordinators.py
48 files changed, 1,920 insertions(+), 1,012 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/15
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 15
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 21: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 21
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 May 2019 18:24:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 23: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4379/
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 23
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Jun 2019 22:46:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 10:
(11 comments)
Thanks for the comments. Please see PS10.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@67
PS8, Line 67: typedef std::shared_ptr<const TBackendDescriptor> BackendDescriptorPtr;
> I think I would prefer if there was something in the name of this and Snaps
Would BackendDescriptorSPtr work for you? I'd like to keep it from getting out-of-hand long, Maybe BeDescSharedPtr would work, too?
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@70
PS8, Line 70: typedef std::unordered_map<std::string, TBackendDescriptor> BackendIdMap;
> nit: std::string in headers here and elsewhere. I guess a "using" is in a h
Done
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@79
PS8, Line 79: // implicitly-defined default and copy constructors.
> Probably worth documenting that we expect this to be copy-constructed.
I'm not sure I'm following your thoughts behind documenting it. Should we document it for users of the struct to mark it as the intended behavior? Or for future developers who want to make changes and need to keep copy-construction supported? For now I assumed the former and added some documentation in comment and the code.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@121
PS8, Line 121: /// steps.
> Can you document thread safety for these functions and the basics of the ca
Done
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@133
PS8, Line 133: /// membership. This callback will only be called when backends are deleted from the
> Valid to call any time before Init()?
Done
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@39
PS8, Line 39: LOG(INFO) << "Starting cluster membership manager";
> Consider DCHECK(TestInfo::is_test());
Thx, TIL.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@188
PS8, Line 188: ckend explicit
> Yeah I like 1) since it makes the comparison more explicit. We should add a
I went with 1. Do you think we should expose the fact that we rely on a host:port combination through the interface? I.e., should if be RemoveExecutor(string krpc_address) or similar instead of passing the full be desc?
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@235
PS8, Line 235: AddLo
> nit: unnecessary std::
Done
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@299
PS8, Line 299: LOG(WARNING) << "Error updating frontend membership snapshot: " << status.GetDetail();
> This method lacks a check to compare the local be desc inside current_backe
Done
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@313
PS8, Line 313: auto it = state.current_backends.find(local_backend_id_);
> Would this consistency check have detected the bug you found? Do we need to
Yes, that's how I found it, a host trying to quiesce itself will hit the CheckConsistency DCHECK after trying to remove its own backend descriptor.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/executor-group.h
File be/src/scheduling/executor-group.h:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/executor-group.h@41
PS8, Line 41: /// host/IP address.
> Worth making explicit that this can be copy-constructed. Either by document
Done
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 10
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 May 2019 23:37:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#6).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaced the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change, it adds maps to store multiple
executor groups.
Testing: This change does not introduce new functionality and the new
class is covered by the existing scheduler unit test and the end to end
tests.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M common/thrift/StatestoreService.thrift
M tests/custom_cluster/test_coordinators.py
26 files changed, 1,162 insertions(+), 923 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/6
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 5:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/runtime/exec-env.cc@454
PS5, Line 454: // Register the ImpalaServer with the cluster membership manager
: cluster_membership_mgr_->SetLocalBeDescFn([server]() {
: return server->GetLocalBackendDescriptor();
: });
: cluster_membership_mgr_->SetUpdateLocalServerFn(
: [server](const ClusterMembershipMgr::BackendAddressSet& current_backends) {
: server->CancelQueriesOnFailedBackends(current_backends);
: });
just thinking out aloud: should we reset the callback functions during teardown (in ~ExecEnv)?
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@54
PS5, Line 54: /// Clients can also register callbacks to receive notifications of changes to the cluster
: /// membership.
this makes it sound like there is a generic way of registering callbacks like we have for statestore can you update this based on the specific callback registrations we have
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@68
PS5, Line 68: pool
nit: executor pool. Is there a description of "executor pools" anywhere?
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@102
PS5, Line 102: then
nit: when
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@112
PS5, Line 112: subscription
nit: subscription.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@117
PS5, Line 117: /// Registers a callback to provide the local backend descriptor.
: void SetLocalBeDescFn(BackendDescriptorPtrFn fn);
:
: /// Registers a callback to notify the local ImpalaServer of changes in the cluster
: /// membership. This callback will only be called when backends are deleted from the
: /// membership.
: void SetUpdateLocalServerFn(UpdateLocalServerFn fn);
:
: /// Registers a callback to notify the local Frontend of changes in the cluster
: /// membership.
: void SetUpdateFrontendFn(UpdateFrontendFn fn);
I know we discussed this offline, but it might be worth documenting in this gerrit review as to why you went with specific callback registration vs having a generic event bus or using the execEnv to call frontend/impalaServer directly them directly
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@165
PS5, Line 165: in
nit: extra word
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@167
PS5, Line 167: / occur in 'current_backends'
nit: outdated comment?
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@184
PS5, Line 184: May be NULL if the set of
: /// backends is fixed.
maybe mention that this is only true in tests
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc@88
PS5, Line 88: update.is_delta && update.topic_entries.empty()
just curious, when can we receive an empty delta
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc@127
PS5, Line 127: const vector<string>& groups = DEFAULT_EXECUTOR_GROUPS;
: for (const string& group : groups) {
: (*new_executor_groups)[group].RemoveExecutor(be_desc);
: }
This kinda implies that an executor can be added to multiple groups. I think in the subsequent patch that introduces the concept of exec groups, every executor will have the group name in their backendDescriptor. maybe add a comment somewhere describing why we are iterating right now, and how this will change
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc@255
PS5, Line 255: auto
nit: const auto&
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc@263
PS5, Line 263: const BackendIdMap::value_type&
nit: const auto&
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc@297
PS5, Line 297: auto
nit: const auto&
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/executor-group.h
File be/src/scheduling/executor-group.h:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/executor-group.h@41
PS5, Line 41: The caller must make sure that the
: /// executor group does not get updated while they hold the reference.
would it make sense to document what is the expected usage pattern for this class? This line is a bit confusing since this is true for any mutable object shared across threads. having some context would be helpful to understand why we are highlighting it here.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/executor-group.h@49
PS5, Line 49: Note that during tests this can include multiple
: /// executors per ip address.
any reason we are specifying this here? the class description already implicitly mentions that there can be multiple executors per IP address.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.h
File be/src/scheduling/scheduler.h:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.h@a334
PS5, Line 334:
we can add something like this back when we introduce exec groups.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.h@145
PS5, Line 145: the global executor
nit: maybe call this an "executor group"?
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.cc@439
PS5, Line 439: // TODO(lars)
marking this in case you missed it
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.cc@654
PS5, Line 654: if (membership_snapshot->local_be_desc.get() == nullptr) {
: return Status("Local backend has not yet started");
: }
can this happen? as you can only submit queries once the impala server is running and accepting new connections, right? Maybe just replace this with a DCheck?
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 04 May 2019 00:50:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 19:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4358/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 19
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 May 2019 00:21:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#17).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaces the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change (IMPALA-8484), it adds maps to store
multiple executor groups.
This change also disables the generation of default operators from the
thrift files and instead adds explicit implementations for the ones that
we rely on. This forces us to explicitly specify comparators when
manipulating containers of thrift structs and will help prevent
accidental bugs.
Testing: This change adds a backend unit test for the new cluster
membership manager. The observable behavior of Impala does not change,
and the existing scheduler unit test and end to end tests should make
sure of that.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/common/thread-debug-info-test.cc
M be/src/experiments/hash-ring-util.cc
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/query-state.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr-test.cc
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/cluster-membership-test-util.cc
A be/src/scheduling/cluster-membership-test-util.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/runtime-profile-test.cc
M be/src/util/uid-util-test.cc
M common/thrift/CMakeLists.txt
M common/thrift/Frontend.thrift
M common/thrift/StatestoreService.thrift
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/custom_cluster/test_coordinators.py
M tests/custom_cluster/test_restart_services.py
49 files changed, 1,974 insertions(+), 1,024 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/17
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 17
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 8:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@67
PS8, Line 67: typedef std::shared_ptr<const TBackendDescriptor> BackendDescriptorPtr;
I think I would prefer if there was something in the name of this and SnapshotPtr that indicated that it was a shared_ptr, so that it was more obvious in other code that it wasn't just a raw pointer. It's probably a matter of taste so I'll leave it up to you.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@70
PS8, Line 70: typedef std::unordered_map<string, TBackendDescriptor> BackendIdMap;
nit: std::string in headers here and elsewhere. I guess a "using" is in a header somewhere.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@79
PS8, Line 79: struct Snapshot {
Probably worth documenting that we expect this to be copy-constructed.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@121
PS8, Line 121: /// Registers a callback to provide the local backend descriptor.
Can you document thread safety for these functions and the basics of the calling sequence (e.g. can be called anytime before or after Init()).
I think it would be good to document invariants in more detail and add DCHECKs as necessary. Otherwise it gets confusing about whether the implementation needs to support various cases and adds cognitive load to people working on the implementation.
I.e.
* Is it valid to call multiple times with different functions. Are there any guarantees that the old callback won't be called after this function returns?
* Is it valid to call with an empty function to deregister a callback.
* Etc.
It looks like in practice it's only ever called once with a non-NULL callback and I don't think we're likely to need to generalise it. So I'd suggested documenting that invariant and adding checks that the function is non-empty as appropriate (https://en.cppreference.com/w/cpp/utility/functional/function/operator_bool)
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@133
PS8, Line 133: /// Returns a read only snapshot of the current cluster membership state.
Valid to call any time before Init()?
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@39
PS8, Line 39: if (statestore_subscriber_ == nullptr) return Status::OK();
Consider DCHECK(TestInfo::is_test());
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@188
PS8, Line 188: RemoveExecutor
> Annoyingly, this call to RemoveExecutor() (and the others below) are wrong.
Yeah I like 1) since it makes the comparison more explicit. We should add a DCHECK too (either making it an invariant that the executor must be present when RemoveExecutor() is called, or returning a bool and DCHECKing here).
I suggested in a comment elsewhere that we could disable auto-generation of the == operator since for most of our thrift types it doesn't make sense to use.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@235
PS8, Line 235: std::
nit: unnecessary std::
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@313
PS8, Line 313: for (const auto& group_it : executor_groups) {
Would this consistency check have detected the bug you found? Do we need to either enhance this check or the testing?
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/executor-group-test.cc
File be/src/scheduling/executor-group-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/executor-group-test.cc@79
PS8, Line 79:
Might be good to have a randomised test for this or ClusterMembershipMgr that adds/removes executors randomly and checks consistency.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/executor-group.h
File be/src/scheduling/executor-group.h:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/executor-group.h@41
PS8, Line 41: class ExecutorGroup {
Worth making explicit that this can be copy-constructed. Either by documentation or by declaring the default copy constructor.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/executor-group.cc
File be/src/scheduling/executor-group.cc:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/executor-group.cc@63
PS8, Line 63: if (find(be_descs.begin(), be_descs.end(), be_desc) == be_descs.end()) {
It would be nice if we could avoid accidentally using the equality operator for TBackendDescriptor. It looks like there's an options for thrift cpp to not generate the equality operators:
diff --git a/common/thrift/CMakeLists.txt b/common/thrift/CMakeLists.txt
index f297292..d7bc014 100644
--- a/common/thrift/CMakeLists.txt
+++ b/common/thrift/CMakeLists.txt
@@ -58,7 +58,7 @@ function(THRIFT_GEN VAR)
# It depends on hive_meta_store, which in turn depends on fb303.
# The java dependency is handled by maven.
# We need to generate C++ src file for the parent dependencies using the "-r" option.
- set(CPP_ARGS ${THRIFT_INCLUDE_DIR_OPTION} --gen cpp:moveable_types -o
+ set(CPP_ARGS ${THRIFT_INCLUDE_DIR_OPTION} --gen cpp:moveable_types,no_default_operators -o
${BE_OUTPUT_DIR})
IF (THRIFT_FILE STREQUAL "beeswax.thrift")
set(CPP_ARGS -r ${CPP_ARGS})
I tried it and there's fallout around TUniqueId and TNetworkAddress - maybe we could disable the codegen option and define custom overloads of the required operators for those two types.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 8
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 May 2019 18:38:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@188
PS8, Line 188: RemoveExecutor
Annoyingly, this call to RemoveExecutor() (and the others below) are wrong. The executor group code uses equality checks to find executors to remove them, and if is_quiescing changes, that won't work.
To solve this I can think of several options:
* 1) Check be_desc.address.port and be_desc.ip_address inside RemoveExecutor (krpc_address could also be a key). This makes the code most flexible.
* 2) DCHECK when calls to RemoveExecutor() don't result in an element being removed (and warn in release builds)
* 3) Switch the call sites here to pass in the existing be_desc instead of the new one.
Both 1) or 3) would fix this, and 2) could provide more guard rails to prevent this in the future. I'm leaning towards 1) because it makes the ExecutorGroup class easier to use and we never should have two be_descs for the same IP:port combination.
What do others think?
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@299
PS8, Line 299: return true;
This method lacks a check to compare the local be desc inside current_backends (if it exists) to local_be_desc.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 8
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 May 2019 17:31:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 22: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4376/
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 22
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Jun 2019 07:27:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 21:
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4370/
The build hit an unrelated, flaky issue: IMPALA-8567
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 21
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Jun 2019 01:47:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 8:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/3135/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 8
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 May 2019 23:44:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 11: Code-Review+1
(7 comments)
Thank you for the changes. This is a vast improvement to this area of the code. I think Bikram should re-review the changes since his +1 because a lot changed here.
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@384
PS10, Line 384: /// removed from the cluster after having been quiesced before, or removed from the
> That's what I meant by "killing" a backend below (L391). I added a comment
Ah great, I missed that.
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@73
PS11, Line 73: /// ClusterMembershipMgr).
indentation is off
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@78
PS11, Line 78: Not
Note
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@80
PS11, Line 80: destructed
I was going to say that this should be "destroyed", but I googled and there's a lot of people on the internet arguing about this, so ¯\_(?)_/¯
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@232
PS11, Line 232: /// It also serves as an example for how craft statestore messages and pass them to
> line has trailing whitespace
how to
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@67
PS8, Line 67: typedef std::shared_ptr<const TBackendDescriptor> BackendDescriptorPtr;
> Any preference here?
BeDescSharedPtr sounds more natural to me.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@79
PS8, Line 79: // implicitly-defined default and copy constructors.
> That makes sense, thanks for the explanation. What do you think about the d
I think that's good.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 11
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 18 May 2019 18:29:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 23:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4385/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 23
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 02 Jun 2019 01:08:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3046/ : 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/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 May 2019 22:47:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3088/ : 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/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 May 2019 21:41:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 18: Code-Review+2
(4 comments)
Addressed the last comments, carrying Bikram's +2.
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.h@210
PS17, Line 210: /// post-recovery grace period. Not exposed to clients and not protected by any locking.
> I guess it kinda is because of the way it is used, but I would still try to
My intention with that comment was the same as Bikram's, the protection is just a coincidence so to speak, but it's not expected to be modified concurrently.
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.cc@86
PS17, Line 86: UpdateMembership
> nit: not a huge deal but I feel this method has become too long, is there a
I agree, it's getting close to 200 lines. It has plenty of comments though and the code is rather linear. We could try to factor out some of the blocks in the core loop and updating the local backend if you think that helps. A lot of the top-level variables and flags are used throughout the method though.
From your +2 I assume you're OK with doing it in a subsequent change? That would allow us to move forward with a handful of dependent changes (which likely won't have to touch this method) and will make the resulting changes easier to review, too.
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.cc@272
PS17, Line 272: if (update_local_server) NotifyLocalServerForDeletedBackend(*new_backend_map);
> I guess this means we call NotifyLocalServerForDeletedBackend() in more cas
Yes, historically we called it for every update. As an attempt to make it more efficient we try not to for some cases but we don't have to be exact about it.
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/statestore/statestore-subscriber.cc@467
PS17, Line 467: MilliSecondsSinceLastRegistration
> nit: I think we dont need to expose this method anymore. we can just get ri
Made it private.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 18
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 May 2019 00:20:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 11:
(18 comments)
Thanks for the comments. Please see my inline replies and PS11. Please also have a brief look at this comment: https://gerrit.cloudera.org/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@67
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@35
PS10, Line 35:
> Maybe it's worth mentioning that it's effectively simulating the statestore
Done
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@48
PS10, Line 48: /// cluster.
> I don't know about the pros/cons of Kudu's RNG but we have some test utilit
Thx. I used kudu::Random because I had used it in the trace_ratio change and I wasn't aware of our own utils. I agree that we should keep it consistent and updated the code.
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@64
PS10, Line 64: /// The list of backends_ that owns all backends.
> It might be helpful to document the states that a backend can be in (and ho
Done
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@86
PS10, Line 86:
> line too long (91 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@86
PS10, Line 86:
> poll?
Done
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@168
PS10, Line 168: // Poll to obtain topic update
> I was trying to think if there were any interesting cases where different C
I agree with your thinking. Independent of the order, processing of updates in each CMM happens sequentially and they don't share state that depends on any racy behavior.
The only detail that could depend on the order in which a backend receives is multiple backends running on a single host which end up in a different order inside a coordinator's executor group. However, that's only relevant to the scheduling and having multiple backends on single host is not supported in production deployments.
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@174
PS10, Line 174: topi
> sent
Done
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@178
PS10, Line 178:
> ?
Done
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@180
PS10, Line 180: /// ClusterMembershipMgr to make the change take effect. The resulting update from the
> line too long (91 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@202
PS10, Line 202: ASSERT_TRUE(is_running || is_quiescing);
> I'm not sure what "driving the interface with the statestore" really means.
Rephrased it, I wanted to say "this is how you use UpdateMembership() as if you're the statestore"
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@358
PS10, Line 358:
> const or constexpr
Done
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@359
PS10, Line 359: T_EQ(
> These are constants, right, so "const double P_ADD"
Done
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@363
PS10, Line 363: }
> empty comment. Actually would be good to document that these are cumulative
Done
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@384
PS10, Line 384: /// removed from the cluster after having been quiesced before, or removed from the
> I feel like we should also be deleting non-quiescing backends, to simulate
That's what I meant by "killing" a backend below (L391). I added a comment to the counter in L367.
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@388
PS10, Line 388: // TODO: Parameterize this test and run with several parameter sets
> line too long (91 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@67
PS8, Line 67: typedef std::shared_ptr<const TBackendDescriptor> BackendDescriptorPtr;
> Would BackendDescriptorSPtr work for you? I'd like to keep it from getting
Any preference here?
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@79
PS8, Line 79: // implicitly-defined default and copy constructors.
> I think both - i think they're pretty tightly coupled anyway. Implicit copy
That makes sense, thanks for the explanation. What do you think about the documentation in PS10?
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@299
PS8, Line 299: LOG(WARNING) << "Error updating frontend membership snapshot: " << status.GetDetail();
> What change was this referring to?
The error here was really in L301, we need to compare 'local_be_desc' to its version inside state.current_backends, not against state.local_be_desc. It is fixed in PS10.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 11
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 May 2019 22:26:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 11: Code-Review+1
(11 comments)
Thanks for the reviews. Please see my inline comments and PS12. It also fixes some fallout from the thrift changes in some tests. Carrying Tim's +1.
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@73
PS11, Line 73: /// ClusterMembershipMgr).
> indentation is off
Done
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@78
PS11, Line 78: Not
> Note
Done
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@120
PS11, Line 120: int num_expected_updates = -1
> nit: unused
Done
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@149
PS11, Line 149: but does not remove the
: /// backend from that list after creating its CMM.
> so those lists are not exclusive, that is, their members can overlap. Can y
I ended up not liking that mixing at all and actually changed the code to enforce that invariant now. I also clarified it in a comment above the list definitions.
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@232
PS11, Line 232: /// It also serves as an example for how craft statestore messages and pass them to
> how to
Done
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@232
PS11, Line 232: /// It also serves as an example for how craft statestore messages and pass them to
> line has trailing whitespace
Done
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@304
PS11, Line 304: mgr
> nit: backend
Done
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@342
PS11, Line 342: // Quiesce them all
> how about Quiesce half of it, to exercise the path where backends later get
Done. I added a loop to the end of this test that quiesces the remaining running backends to make sure that executor groups can scale back to 0 element.
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@391
PS11, Line 391: P_SHUTDOWN
> nit: maybe call it P_QUIESCE to be consistent
Done
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr.cc@350
PS11, Line 350: : is_quiescing"
> nit: maybe print here and below what the value is for group_be.is_quiescing
Done. Note that the warnings in L333 and L338 print the state for the backend in the executor group, but I agree that it could have been more clear and added more to the output.
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/executor-group-test.cc
File be/src/scheduling/executor-group-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/executor-group-test.cc@61
PS11, Line 61: executor_group.RemoveExecutor(MakeBackendDescriptor(2));
> will the LOG DFATAL result in an exit here and cause the test to fail?
I'm not sure I understand the question. In this particular case it won't because we remove a backend that we added in the previous line (MakeBackendDescriptor() creates identical backends when called with the same index parameter).
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 11
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 May 2019 20:40:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/runtime/exec-env.cc@454
PS5, Line 454: // Register the ImpalaServer with the cluster membership manager
: cluster_membership_mgr_->SetLocalBeDescFn([server]() {
: return server->GetLocalBackendDescriptor();
: });
: cluster_membership_mgr_->SetUpdateLocalServerFn(
: [server](const ClusterMembershipMgr::BackendAddressSet& current_backends) {
: server->CancelQueriesOnFailedBackends(current_backends);
: });
> Thanks the DCHECK would definitely help. I was also just thinking of a situ
I think we have this problem in every place where we call ExecEnv::impala_server(). The server doesn't unregister itself and the ExecEnv doesn't own it. Furthermore that holds true for all code that depends on ExecEnv, since it doesn't implement a d'tor itself and we rely on GetInstance() to return a valid instance.
http://gerrit.cloudera.org:8080/#/c/13207/6/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/6/be/src/scheduling/cluster-membership-mgr.h@54
PS6, Line 54: ions of changes to the cluster
: /// membership.
> should this also be after a TODO tag?
I don't feel strongly here. The class already has code in place that supports this, and consumers should expect it. However, it's currently not used by any client code. The TODO below in L57 captures the intent to add the missing parts in IMPALA-8484, so I removed the sentence altogether.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 May 2019 20:51:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#18).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaces the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change (IMPALA-8484), it adds maps to store
multiple executor groups.
This change also disables the generation of default operators from the
thrift files and instead adds explicit implementations for the ones that
we rely on. This forces us to explicitly specify comparators when
manipulating containers of thrift structs and will help prevent
accidental bugs.
Testing: This change adds a backend unit test for the new cluster
membership manager. The observable behavior of Impala does not change,
and the existing scheduler unit test and end to end tests should make
sure of that.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/common/thread-debug-info-test.cc
M be/src/experiments/hash-ring-util.cc
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/query-state.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr-test.cc
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/cluster-membership-test-util.cc
A be/src/scheduling/cluster-membership-test-util.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/runtime-profile-test.cc
M be/src/util/uid-util-test.cc
M common/thrift/CMakeLists.txt
M common/thrift/Frontend.thrift
M common/thrift/StatestoreService.thrift
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/custom_cluster/test_coordinators.py
M tests/custom_cluster/test_restart_services.py
49 files changed, 1,977 insertions(+), 1,027 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/18
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 18
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 6: Code-Review+1
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/runtime/exec-env.cc@454
PS5, Line 454: impala_server_ = server;
: // Register the ImpalaServer with the cluster membership manager
: cluster_membership_mgr_->SetLocalBeDescFn([server]() {
: return server->GetLocalBackendDescriptor();
: });
: cluster_membership_mgr_->SetUpdateLocalServerFn(
: [server](const ClusterMembershipMgr::BackendAddressSet& current_backends) {
:
> These rely only on the server still being alive, not the ExecEnv, so the ri
Thanks the DCHECK would definitely help. I was also just thinking of a situation during teardown where there can be a really small window where the impala_server got destroyed and the subscriber receives an update on membership and ClusterMembershipManager tries to call the function pointer. But I think it doesnt matter because impala is already shutting down at that point
http://gerrit.cloudera.org:8080/#/c/13207/6/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/6/be/src/scheduling/cluster-membership-mgr.h@54
PS6, Line 54: as indicated by their backend
: /// descriptor.
should this also be after a TODO tag?
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@117
PS5, Line 117: ClusterMembershipMgr(std::string local_backend_id, StatestoreSubscriber* subscriber);
:
: /// Initializes instances of this class. This only sets up the statestore subscription.
: /// Callbacks to the local ImpalaServer and Frontend must be registered in separate
: /// steps.
: Status Init();
:
: /// Registers a callback to provide the local backend descriptor.
: void SetLocalBeDescFn(BackendDescriptorPtrFn fn);
:
: /// Registers a callback to notify the local I
> Both clients have different needs: The ImpalaServer only needs to learn abo
Thanks, that sums it up perfectly!
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc@255
PS5, Line 255: cons
> Done. The const is redundant but I added it for readability. (See https://g
oh interesting! I suspected that too, but i checked in eclipse and it was not automatically deducing the const for some reason.
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.cc@654
PS5, Line 654: // This can happen in the short time period after the ImpalaServer has finished
: // starting up (which makes the local backend available) and the next statestore
:
> There's a race between the ImpalaServer starting up and the statestore topi
I see. Thanks for the explanation
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 May 2019 18:11:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 5:
(22 comments)
Thx for the review, please see my comments and PS5.
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.h@96
PS2, Line 96: typedef std::function<BackendDescriptorPtr()> BackendDescriptorPtrFn;
> Should we document that no locks are held during by the calling thread of t
Done. It's a bit fuzzy where to put the bulk of the comments for the callbacks and they're currently spread across the member, function, and typedef. Let me know if you'd prefer to collect them in a single place (which one) and refer to it at the other locations.
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@94
PS2, Line 94:
> nit: no
Done
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@98
PS2, Line 98:
> I feel like a lot of these messages could actually be VLOG(2) or even VLOG(
I agree that they help, I used -vmodule already while developing this so I changed them to VLOG(1).
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@108
PS2, Line 108: } else {
> We're assuming that only one thread executes the below code at a time, righ
Yes, that's the statestore contract as far as I understand, that the membership callback needs to return before being called again. DFAKE_SCOPED_LOCK_THREAD_LOCKED looks like it enforces that the actual same thread runs the method every time. I think we want DFAKE_SCOPED_LOCK, but let me know if I misunderstood your intent.
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@113
PS2, Line 113: }
> Conditional fits on one line?
Done
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@145
PS2, Line 145: if (!status.ok()) {
> This seems like something we should definitely log at INFO or probably even
Done
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@152
PS2, Line 152: // descriptor as part of the statestore update. If it is empty, then either that
> Same here
Done
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@201
PS2, Line 201:
> typo
Done
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@242
PS2, Line 242: Status status = thrift_serializer_.SerializeToString(&local_be_desc, &item.value);
> It seems like this really should be a LOG(FATAL) or a CHECK since we never
Done, I'm happy to fix these things while we're here.
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@252
PS2, Line 252: const BackendIdMap& current_backends) {
> This is O(n^2) where n in the cluster size, in some sense because we potent
Yes, this logic was there before, that on every statestore update the impalad would construct a set (not even unordered) and check it for cancellation. An easy optimization we can do is only call this method if a backend was removed.
Unfortunately we cannot do that for the Frontend notification below.
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@282
PS2, Line 282:
> nit: whitespace
Done
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@283
PS2, Line 283: bool ClusterMembershipMgr::NeedsLocalBackendUpdate(const Snapshot& state,
> I don't personally like the redundant .get() here but it is a matter of tas
I got used to it and don't really mind it, so I'll keep it for consistency (hard to tell how often we rely on the cast with a simple grep). I'm happy to try and change our recommendation on it though.
$Â git grep -E "get.*= nullptr" | wc -l
131
http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@312
PS2, Line 312: if (group_be.is_quiescing) {
> return false here and on l308?
Done
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h
File be/src/scheduling/executor-group.h:
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@38
PS3, Line 38: typedef std::vector<IpAddr> IpAddrs;
> Explicit constructors?
Removed it.
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@41
PS3, Line 41: /// the host is actually contained in executor_map_. The caller must make sure that the
> Any reason this shouldn't be a vector? That's generally a better default. I
This is probably a leftover from when we had concurrent updates to the state during scheduling. Made it a vector.
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@43
PS3, Line 43: /// membership manager returns immutable snapshots to guarantee this.
> What's the thread safety story for the returned list?
Added a comment. Strictly speaking it's the responsibility of the users to not update the executors while holding a reference. The CMM ensures this using the immutable snapshots.
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@47
PS3, Line 47: IpAddrs GetAllExecutorIps() const;
> Can you document whether these append to the vectors or replace the content
Switched to return-by-value
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@49
PS3, Line 49: /// Returns all executor backends. Note that during tests this can include multiple
> I think these could do with a basic description. There may be some details
Done
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@62
PS3, Line 62: /// be nullptr if the caller only wants to check whether the lookup succeeds. Use this
> What happens if the executor is removed with RemoveExecutor()?
Added a comment, after switching to a vector, the user must make sure that the group doesn't change. Should we apply some pattern to make the group immutable? I don't know of a good way to forbid methods from running on a non-const reference/pointer.
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@65
PS3, Line 65: bool LookUpExecutorIp(const Hostname& hostname, IpAddr* ip) const;
> Maybe worth documenting lifetime/thread safety of this?
Done
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@71
PS3, Line 71: const TBackendDescriptor* LookUpBackendDesc(const TNetworkAddress& host) const;
> Maybe a good time to switch these to std::unordered_map?
Done
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.cc
File be/src/scheduling/executor-group.cc:
http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.cc@109
PS3, Line 109:
> LIKELY annotation probably isn't necessary. Ok to leave.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 May 2019 21:50:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#5).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaced the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change, it adds maps to store multiple
executor groups.
Testing: This change does not introduce new functionality and the new
class is covered by the existing scheduler unit test and the end to end
tests.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M common/thrift/StatestoreService.thrift
25 files changed, 1,134 insertions(+), 914 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/5
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 13:
(2 comments)
Thanks for the reviews! Addressed the remaining comments in PS14, will rebase the change next.
http://gerrit.cloudera.org:8080/#/c/13207/13/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/13/be/src/scheduling/cluster-membership-mgr-test.cc@400
PS13, Line 400: num_expected_alive
> use 'idx' here instead as this would fail if NUM_BACKENDS/2 is odd, eg for
Oh, that was an annoying mistake. I fixed it properly and made sure it works correctly for N=10.
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr.cc@256
PS11, Line 256: update.is_delta = true;
> why do we set this to true explicitly?
Technically because it's the right thing to do: This coordinator only provides a delta to the global state. The statestore doesn't require this to be set, but doing so allows us to use the update in tests without modifying it, e.g. to pass it to other ClusterMembershipMgr instances.
I added a comment
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 13
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 May 2019 17:33:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 13:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3407/ : 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/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 13
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 May 2019 16:56:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 20: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 20
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 May 2019 18:17:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 22: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 22
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Jun 2019 01:47:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 12: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13207/12/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/12/be/src/scheduling/cluster-membership-mgr-test.cc@391
PS12, Line 391: DeleteBackend(running_.front());
This pattern of deleting the front of the vector ends up being O(n^2), since the subsequent elements have to be bumpe each time. I don't think this is a big deal with the cluster sizes currently but might burn someone if they tried to make it work for larger fake clusters.
I don't think it's a big enough deal to do anything about. I wonder if switching to a deque would be an easy fix.
Ok to ignore.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 12
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 May 2019 03:54:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 17:
(1 comment)
PS 15 and 16 are the rebase, PS 17 has additional changes to address IMPALA-7665.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@67
PS8, Line 67: class ClusterMembershipMgr {
> BeDescSharedPtr sounds more natural to me.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 17
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 May 2019 21:16:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#14).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaces the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change (IMPALA-8484), it adds maps to store
multiple executor groups.
This change also disables the generation of default operators from the
thrift files and instead adds explicit implementations for the ones that
we rely on. This forces us to explicitly specify comparators when
manipulating containers of thrift structs and will help prevent
accidental bugs.
Testing: This change adds a backend unit test for the new cluster
membership manager. The observable behavior of Impala does not change,
and the existing scheduler unit test and end to end tests should make
sure of that.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/common/thread-debug-info-test.cc
M be/src/experiments/hash-ring-util.cc
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/query-state.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr-test.cc
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/cluster-membership-test-util.cc
A be/src/scheduling/cluster-membership-test-util.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/runtime-profile-test.cc
M be/src/util/uid-util-test.cc
M common/thrift/CMakeLists.txt
M common/thrift/Frontend.thrift
M common/thrift/StatestoreService.thrift
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/custom_cluster/test_coordinators.py
48 files changed, 1,917 insertions(+), 1,011 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/14
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 14
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#11).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaces the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change (IMPALA-8484), it adds maps to store
multiple executor groups.
This change also disables the generation of default operators from the
thrift files and instead adds explicit implementations for the ones that
we rely on. This forces us to explicitly specify comparators when
manipulating containers of thrift structs and will help prevent
accidental bugs.
Testing: This change adds a backend unit test for the new cluster
membership manager. The observable behavior of Impala does not change,
and the existing scheduler unit test and end to end tests should make
sure of that.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/query-state.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr-test.cc
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/cluster-membership-test-util.cc
A be/src/scheduling/cluster-membership-test-util.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/uid-util-test.cc
M common/thrift/CMakeLists.txt
M common/thrift/Frontend.thrift
M common/thrift/StatestoreService.thrift
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/custom_cluster/test_coordinators.py
44 files changed, 1,867 insertions(+), 1,008 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/11
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 11
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 7:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/3133/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 7
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 May 2019 23:06:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 10:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/3258/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 10
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 May 2019 00:27:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3045/ : 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/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 4
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 May 2019 22:25:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 22:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4376/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 22
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Jun 2019 01:47:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 18:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3453/ : 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/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 18
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 May 2019 01:00:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 21:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4370/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 21
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 May 2019 18:24:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
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/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaces the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also generalizes the fix for IMPALA-7665: updates from the
statestore to the cluster membership topic are only made visible to the
rest of the local server after a post-recovery grace period has elapsed.
As part of this the flag
'failed_backends_query_cancellation_grace_period_ms' is replaced with
'statestore_subscriber_recovery_grace_period_ms'. To tell the initial
startup from post-recovery, a new metric
'statestore-subscriber.num-connection-failures' is exposed by the
daemon, which tracks the total number of connection failures to the
statestore over the lifetime process lifetime.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change (IMPALA-8484), it adds maps to store
multiple executor groups.
This change also disables the generation of default operators from the
thrift files and instead adds explicit implementations for the ones that
we rely on. This forces us to explicitly specify comparators when
manipulating containers of thrift structs and will help prevent
accidental bugs.
Testing: This change adds a backend unit test for the new cluster
membership manager. The observable behavior of Impala does not change,
and the existing scheduler unit test and end to end tests should make
sure of that.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Reviewed-on: http://gerrit.cloudera.org:8080/13207
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/common/thread-debug-info-test.cc
M be/src/experiments/hash-ring-util.cc
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/query-state.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr-test.cc
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/cluster-membership-test-util.cc
A be/src/scheduling/cluster-membership-test-util.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/runtime-profile-test.cc
M be/src/util/uid-util-test.cc
M common/thrift/CMakeLists.txt
M common/thrift/Frontend.thrift
M common/thrift/StatestoreService.thrift
M common/thrift/metrics.json
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/custom_cluster/test_coordinators.py
M tests/custom_cluster/test_restart_services.py
50 files changed, 1,994 insertions(+), 1,027 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 24
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 23:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4379/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 23
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Jun 2019 17:14:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 10:
(15 comments)
Thanks for your improvements - I think this is good. I really like the test, it'll be valuable in future.
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@35
PS10, Line 35: /// handling code in 4 subsequently more sophisticated ways:
Maybe it's worth mentioning that it's effectively simulating the statestore except updates are done serially
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@48
PS10, Line 48: rng_.Reset(seed);
I don't know about the pros/cons of Kudu's RNG but we have some test utilities that handle seeding and allow overriding with an env variable - RandTestUtil::SeedRng(). I'd be inclined to keep things consistent within our test code.
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@64
PS10, Line 64: /// Backends that are created but offline, i.e. they have no ClusterMembershipMgr and
It might be helpful to document the states that a backend can be in (and how that's represented in terms of membership in these lists).
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@86
PS10, Line 86: pull
poll?
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@168
PS10, Line 168: // Broadcast to all other backends
I was trying to think if there were any interesting cases where different CMMs saw deltas with different timing, e.g. two executors quiesce simultaneously so don't see the other's quiescence before they mark themselves as quiesced. The test framework I think is simulating serial updates but the statestore polls for updates in parallel.
I think this is unimportant since state of other backends doesn't affect the deltas pushed out in any way and each key is single writer, but just wanted to double check that you agreed with my thinking.
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@174
PS10, Line 174: send
sent
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@178
PS10, Line 178: // TODO Make these a function
?
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@202
PS10, Line 202: /// statestore directly.
I'm not sure what "driving the interface with the statestore" really means.
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@358
PS10, Line 358:
const or constexpr
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@359
PS10, Line 359: p_add
These are constants, right, so "const double P_ADD"
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@363
PS10, Line 363: //
empty comment. Actually would be good to document that these are cumulative counts.
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@384
PS10, Line 384: if (p < p_delete && !quiescing_.empty()) {
I feel like we should also be deleting non-quiescing backends, to simulate non-graceful failures. I don't think it adds much complexity (famous last words).
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@79
PS8, Line 79: // implicitly-defined default and copy constructors.
> I'm not sure I'm following your thoughts behind documenting it. Should we d
I think both - i think they're pretty tightly coupled anyway. Implicit copy constructors for complex structs are a bit of a code smell - I'd be suspicious if I came across this code and think that maybe it was accidentally copying things. I can also see devs accidentally breaking copy construction if they didn't realise it was meant to be used that way.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@188
PS8, Line 188: ckend explicit
> I went with 1. Do you think we should expose the fact that we rely on a hos
I guess I don't see a reason why we need to change the interface, so this is fine.
http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@299
PS8, Line 299: LOG(WARNING) << "Error updating frontend membership snapshot: " << status.GetDetail();
> Done
What change was this referring to?
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 10
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 May 2019 01:06:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 3:
I was a bit too eager in removing code and includes, but PS3 passes the core tests in a private jenkins job.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 May 2019 16:07:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13207/6/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/6/be/src/scheduling/cluster-membership-mgr.h@125
PS6, Line 125: void SetLocalBeDescFn(BackendDescriptorPtrFn fn);
> Unfortunately I don't think these Set*Fn() functions are really thread-safe
I ended up with the second approach. Deferring Init() of the ClusterMembershipMgr until the ImpalaServer registers itself with the ExecEnv also would have meant to defer the initialization of the Scheduler and AdmissionController and I found the additional synchronization easier to reason about than changing our initialization order and interlacing it with ImpalaServer::Start(). Let me know if you feel that that's what we should do.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 May 2019 21:28:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaced the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change, it adds maps to store multiple
executor groups.
Testing: This change does not introduce new functionality and the new
class is covered by the existing scheduler unit test and the end to end
tests.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M common/thrift/StatestoreService.thrift
25 files changed, 1,131 insertions(+), 914 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/4
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 4
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#12).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaces the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change (IMPALA-8484), it adds maps to store
multiple executor groups.
This change also disables the generation of default operators from the
thrift files and instead adds explicit implementations for the ones that
we rely on. This forces us to explicitly specify comparators when
manipulating containers of thrift structs and will help prevent
accidental bugs.
Testing: This change adds a backend unit test for the new cluster
membership manager. The observable behavior of Impala does not change,
and the existing scheduler unit test and end to end tests should make
sure of that.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/common/thread-debug-info-test.cc
M be/src/experiments/hash-ring-util.cc
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/query-state.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr-test.cc
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/cluster-membership-test-util.cc
A be/src/scheduling/cluster-membership-test-util.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/runtime-profile-test.cc
M be/src/util/uid-util-test.cc
M common/thrift/CMakeLists.txt
M common/thrift/Frontend.thrift
M common/thrift/StatestoreService.thrift
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/custom_cluster/test_coordinators.py
48 files changed, 1,910 insertions(+), 1,010 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/12
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 12
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#10).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaces the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change (IMPALA-8484), it adds maps to store
multiple executor groups.
This change also disables the generation of default operators from the
thrift files and instead adds explicit implementations for the ones that
we rely on. This forces us to explicitly specify comparators when
manipulating containers of thrift structs and will help prevent
accidental bugs.
Testing: This change adds a backend unit test for the new cluster
membership manager. The observable behavior of Impala does not change,
and the existing scheduler unit test and end to end tests should make
sure of that.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/query-state.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr-test.cc
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/cluster-membership-test-util.cc
A be/src/scheduling/cluster-membership-test-util.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/uid-util-test.cc
M common/thrift/CMakeLists.txt
M common/thrift/Frontend.thrift
M common/thrift/StatestoreService.thrift
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/custom_cluster/test_coordinators.py
44 files changed, 1,831 insertions(+), 1,008 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/10
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 10
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13207/12/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/12/be/src/scheduling/cluster-membership-mgr-test.cc@391
PS12, Line 391: DeleteBackend(running_.front());
> This pattern of deleting the front of the vector ends up being O(n^2), sinc
Done. Note that in DeleteBackend() we also ensure that the backend was removed successfully which still results in O(n^2) complexity. Let me know if you'd like to remove the check there, too.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 12
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 May 2019 16:53:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 19: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4358/
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 19
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 May 2019 08:49:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 21: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4370/
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 21
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Jun 2019 00:41:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 13: Code-Review+2
(2 comments)
Looks good. Feel free to +2 this after you've addressed these comments.
http://gerrit.cloudera.org:8080/#/c/13207/13/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/13/be/src/scheduling/cluster-membership-mgr-test.cc@400
PS13, Line 400: num_expected_alive
use 'idx' here instead as this would fail if NUM_BACKENDS/2 is odd, eg for NUM_BACKENDS = 10
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr.cc@256
PS11, Line 256: update.is_delta = true;
why do we set this to true explicitly?
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 13
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 May 2019 23:15:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 16:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3449/ : 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/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 16
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 May 2019 21:32:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 17: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.h@210
PS17, Line 210: /// post-recovery grace period. Not exposed to clients and not protected by any locking.
> This is only accessed by UpdateMembership(),right? I.e. "protected" by upda
I guess it kinda is because of the way it is used, but I would still try to avoid mentioning that it "is protected" by the lock, because that might imply that it can be modified concurrently.
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.cc@86
PS17, Line 86: UpdateMembership
nit: not a huge deal but I feel this method has become too long, is there anyway we can decompose it?
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:
http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/statestore/statestore-subscriber.cc@467
PS17, Line 467: MilliSecondsSinceLastRegistration
nit: I think we dont need to expose this method anymore. we can just get rid of it altogether or just make it private.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 17
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 May 2019 00:07:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/3024/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 May 2019 19:01:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@232
PS11, Line 232: /// It also serves as an example for how craft statestore messages and pass them to
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 11
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 May 2019 22:26:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#13).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaces the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change (IMPALA-8484), it adds maps to store
multiple executor groups.
This change also disables the generation of default operators from the
thrift files and instead adds explicit implementations for the ones that
we rely on. This forces us to explicitly specify comparators when
manipulating containers of thrift structs and will help prevent
accidental bugs.
Testing: This change adds a backend unit test for the new cluster
membership manager. The observable behavior of Impala does not change,
and the existing scheduler unit test and end to end tests should make
sure of that.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/common/thread-debug-info-test.cc
M be/src/experiments/hash-ring-util.cc
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/rpc/thrift-util-test.cc
M be/src/rpc/thrift-util.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/query-state.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr-test.cc
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/cluster-membership-test-util.cc
A be/src/scheduling/cluster-membership-test-util.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/container-util.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/runtime-profile-test.cc
M be/src/util/uid-util-test.cc
M common/thrift/CMakeLists.txt
M common/thrift/Frontend.thrift
M common/thrift/StatestoreService.thrift
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/custom_cluster/test_coordinators.py
48 files changed, 1,915 insertions(+), 1,011 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/13
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 13
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3038/ : 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/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 May 2019 17:01:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 19: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 19
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 May 2019 00:21:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 23: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 23
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 02 Jun 2019 06:38:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 7:
(3 comments)
Had a few comments, still need to complete review
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/runtime/exec-env.cc@454
PS5, Line 454: impala_server_ = server;
: // Register the ImpalaServer with the cluster membership manager
: cluster_membership_mgr_->SetLocalBeDescFn([server]() {
: return server->GetLocalBackendDescriptor();
: });
: cluster_membership_mgr_->SetUpdateLocalServerFn(
: [server](const ClusterMembershipMgr::BackendAddressSet& current_backends) {
:
> Thanks the DCHECK would definitely help. I was also just thinking of a situ
We shouldn't be running ~ExecEnv outside of backend tests AFAIK.
http://gerrit.cloudera.org:8080/#/c/13207/6/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/13207/6/be/src/scheduling/cluster-membership-mgr.h@125
PS6, Line 125: /// membership. This callback will only be called when backends are deleted from the
Unfortunately I don't think these Set*Fn() functions are really thread-safe, and it looks like Init() registers ClusterMembershipMgr with the statestore subscriber, so it's possible that a statestore update could race with these functions getting updated.
It's probably unlikely to lead to a bug so long as these functions are always set from non-NULL to NULL, but it hard to reason about given that std::function isn't just a simple pointer, so the the reader could see a "torn" state. I think it would also trip TSAN, etc.
I think we could solve it two ways:
* Require that all the callbacks are registered before Init() is called.
* Add some synchronisations around the function pointers. I don't think any of this is on a critical path, so I wouldn't be too concerned about a simple SpinLock.
I think the first would be cleaner, although I understand there are some initialisation order issues between ExecEnv and ImpalaServer. Maybe we can defer initialisation of ClusterMembershipMgr until after SetImpalaServer() is called or something like that?
http://gerrit.cloudera.org:8080/#/c/13207/6/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13207/6/be/src/scheduling/cluster-membership-mgr.cc@54
PS6, Line 54: void ClusterMembershipMgr::SetLocalBeDescFn(BackendDescriptorPtrFn fn) {
Maybe DCHECK that it's non-NULL here and in the other functions below.
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 7
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 May 2019 20:51:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13207
to look at the new patch set (#7).
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
IMPALA-8460: Simplify cluster membership management
This change adds a class to track cluster membership called
ClusterMembershipMgr. It replaced the logic that was partially
duplicated between the ImpalaServer and the Coordinator and makes sure
that the local backend descriptor is consistent (IMPALA-8469).
The ClusterMembershipMgr maintains a view of the cluster membership and
incorporates incoming updates from the statestore. It also registers the
local backend with the statestore after startup. Clients can obtain a
consistent, immutable snapshot of the current cluster membership from
the ClusterMembershipMgr. Additionally, callbacks can be registered to
receive notifications of cluster membership changes. The ImpalaServer
and Frontend use this mechanism.
This change also unifies the naming of executor-related classes, in
particular it renames "BackendConfig" to "ExecutorGroup". In
anticipation of a subsequent change, it adds maps to store multiple
executor groups.
Testing: This change does not introduce new functionality and the new
class is covered by the existing scheduler unit test and the end to end
tests.
Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
---
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/common/logging.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/CMakeLists.txt
D be/src/scheduling/backend-config-test.cc
D be/src/scheduling/backend-config.cc
D be/src/scheduling/backend-config.h
A be/src/scheduling/cluster-membership-mgr.cc
A be/src/scheduling/cluster-membership-mgr.h
A be/src/scheduling/executor-group-test.cc
A be/src/scheduling/executor-group.cc
A be/src/scheduling/executor-group.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M common/thrift/StatestoreService.thrift
M tests/custom_cluster/test_coordinators.py
26 files changed, 1,159 insertions(+), 923 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13207/7
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 7
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8460: Simplify cluster membership management
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management
......................................................................
Patch Set 10:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@86
PS10, Line 86: // The empty delta is used to pull subscribers for updates without sending new changes.
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@180
PS10, Line 180: bool is_quiescing = find(quiescing_.begin(), quiescing_.end(), be) != quiescing_.end();
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@388
PS10, Line 388: quiescing_.erase(remove(quiescing_.begin(), quiescing_.end(), be), quiescing_.end());
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/13207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3
Gerrit-Change-Number: 13207
Gerrit-PatchSet: 10
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 May 2019 23:37:39 +0000
Gerrit-HasComments: Yes