You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Huaisi Xu (Code Review)" <ge...@cloudera.org> on 2016/06/06 22:36:17 UTC
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Huaisi Xu has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3320
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
IMPALA-3613: Do not update unregistered subscribers
In DoSubscriberUpdate(), previously we check a subscriber
by its hostname. This fix checks the unique registration id
as well so that impala no longer sends RPCs to unregistered
subscribers.
Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
2 files changed, 19 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/20/3320/1
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#2).
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
IMPALA-3613: Do not update unregistered subscribers
In DoSubscriberUpdate(), previously we check a subscriber
by its hostname. This fix checks the unique registration id
as well so that impala no longer sends RPCs to unregistered
subscribers.
Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
2 files changed, 21 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/20/3320/2
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3320/3/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
Line 615: // Check if this subscriber was re-registered
unregistered or re-registered
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has abandoned this change.
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
Patch Set 3:
Please update to using the new gerrit project, "Impala-ASF".
Instructions are here:
https://cwiki.apache.org/confluence/display/IMPALA/How+to+switch+to+Apache-hosted+git
Pushes to this project will be disabled on October 1.
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#3).
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
IMPALA-3613: Do not update unregistered subscribers
In DoSubscriberUpdate(), previously we check a subscriber
by its hostname. This fix checks the unique registration id
as well so that impala no longer sends RPCs to unregistered
subscribers.
Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
2 files changed, 28 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/20/3320/3
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
Patch Set 2:
(1 comment)
testing is done by setting hearbeat interval to 10s, and subscriber's timeout to be 1s. after ~1 hour everything looks good to me on a 3 nodes cluster(local dev).
http://gerrit.cloudera.org:8080/#/c/3320/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
Line 714: LOG(INFO) << "Next " << (is_heartbeat ? "heartbeat" : "update") << " deadline for: "
I will rever this...
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
Patch Set 2:
(2 comments)
> (2 comments)
>
> Did you check the size of the statestore's subscriber queue?
>
> After 1 hour the subscribers should have sent ~60 * 60 * 3
> registrations, which is 10800, but even with one thread would have
> sent ~3600 heartbeats at one every six seconds. So the registration
> queue could still be filling up.
>
> I think it would be useful to have the heartbeat frequency be 30
> minutes in your test. Then after the heartbeats are sent the
> statestore should have almost completely cleaned out the queue, and
> you can check that the queue size is small.
>
> That means adding some way to see the size of the queue. You could
> just print it periodically in your test environment for starters.
I did print that and heartbeat in my test. queue size remained close to 0 all the time instead of gradually increasing over time.(I forgot why though..: )) heartbeat never repeat unexpectedly after the change.
http://gerrit.cloudera.org:8080/#/c/3320/2/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
PS2, Line 362: the second entry is the subscriber to send it to.
> comment needs updating
I think it is correct to say that "the second entry is the subscriber to send it to"... if we think the subscriber more unique.
Line 363: typedef std::pair<int64_t, std::pair<SubscriberId, TUniqueId>> ScheduledSubscriberUpdate;
> nit: long line
so close(1 character). do we really have to follow 90 characters in this case?
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
Patch Set 2:
(2 comments)
Did you check the size of the statestore's subscriber queue?
After 1 hour the subscribers should have sent ~60 * 60 * 3 registrations, which is 10800, but even with one thread would have sent ~3600 heartbeats at one every six seconds. So the registration queue could still be filling up.
I think it would be useful to have the heartbeat frequency be 30 minutes in your test. Then after the heartbeats are sent the statestore should have almost completely cleaned out the queue, and you can check that the queue size is small.
That means adding some way to see the size of the queue. You could just print it periodically in your test environment for starters.
http://gerrit.cloudera.org:8080/#/c/3320/2/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
PS2, Line 362: the second entry is the subscriber to send it to.
comment needs updating
Line 363: typedef std::pair<int64_t, std::pair<SubscriberId, TUniqueId>> ScheduledSubscriberUpdate;
nit: long line
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
Patch Set 2:
let me check again
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
Patch Set 2:
Hm - isn't that unexpected if the queue size doesn't increase over time? Shouldn't the subscribers continually append to the heartbeat queue?
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
Patch Set 2:
I think that is because impalad re-register at most every 5s(recoverymodechecker).
and every 10s at most 8(include catalog) can register, and these can be handled(dropped) easily by 10 update threads. Even with waiting. I will give a patch that returns immediately
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3613: Do not update unregistered subscribers
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3613: Do not update unregistered subscribers
......................................................................
Patch Set 2:
> Hm - isn't that unexpected if the queue size doesn't increase over
> time? Shouldn't the subscribers continually append to the heartbeat
> queue?
I checked and I think that is because the recoverymodechecker has a 5s checking circle, which I did not change, so statestore is able to clean everything. The point of this patch is to avoid resending update/heartbeat to re-registered node. Does this explain you question?
--
To view, visit http://gerrit.cloudera.org:8080/3320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic573d257b8171bfcab33a1f72de1399be7db10fd
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No