You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org> on 2018/07/26 00:09:05 UTC
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Pooja Nilangekar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11052
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
This patch collects the recent heartbeat timestamp of a subscriber
in 60 second intervals. It publishes this timsetamp to the
/subscribers page of the statestore and also adds it to the log.
Testing: Manually inspected the Web UI and statestore logs to
verify that the recent heartbeat timestamp for each subscriber is
updated periodically.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M www/statestore_subscribers.tmpl
3 files changed, 37 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11052/1
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 1
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
Patch Set 1:
Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/68/
Running initial code review checks. This is experimental - please report any issues to tarmstrong@cloudera.com or on this JIRA: IMPALA-7317
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 1
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jul 2018 00:09:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.h@447
PS8, Line 447: Topics priority_subscribed_topics_;
> To incorporate the changes I changes the last_heartbeat_ts_ to kudu::MonoTi
Does the worker thread which calls RefreshHeartbeatTimestamp also hold subscriber_lock_ while doing so? If not, I think it's technically required to make it atomic (or ensure that the caller does hold that lock).
That's the case even though the underlying variable here happens to be int64 and therefore atomic "naturally" on x86. Accessing it without synchronization ends up in murky territory around what optimizations compilers are allowed to do, etc, and also means that if we ever implement TSAN support for running tests we'd see a bunch of warnings about incorrect synchronization.
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@389
PS8, Line 389:
> Yes, it does occur (especially during initialization). I changed it from mi
Even though the resolution stored is in nanoseconds it's possible that the clock source doesn't have the same resolution (ie it may step with some "coarseness" rather than being accurate to the nano). I think the particular clock implementation we use on Linux are fine, but baking that kind of timing assumption in here seems like we might just get a really surprising failure at some point down the road.
I'd also think about this from the other direction: let's say we did accidentally update it twice at the same microsecond. Would it be a problem? Since we're just using this for reporting freshness of heartbeats, and not relying on it being increasing-only or something, I don't think it's worth being strict.
http://gerrit.cloudera.org:8080/#/c/11052/9/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/9/be/src/statestore/statestore.cc@451
PS9, Line 451: initialized_ = true;
shouldn't this actually go at the end of the function? ie if the server fails to start and returns a bad Status here, it's acceptable to destruct the Statestore - it's only once we've started the new thread that it enters the "unstoppable" state.
I think with it in this position, you might trigger the CHECK failure if the SSL configuration were incorrect or the port was already bound, whereas maybe we'd want to edit in a more graceful fashion.
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 9
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 24 Aug 2018 22:40:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 8:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.h@420
PS8, Line 420: /// Sets the subscriber's last heartbeat timestamp to the given value.
nit: can you clarify what the reference point for this timestamp is? ie is this a wall time or monotonic time? You might consider using the kudu::MonoTime class here to make that clear in lieu of a comment (not sure if you have a similar wrapper, but that should already be imported in kudu/util)
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.h@445
PS8, Line 445: microseconds
same as above
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.h@538
PS8, Line 538: AtomicBool
why does this need to be atomic? shouldn't we assume that initialization is single-threaded and the object should not be exposed to other threads until after initialization?
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@77
PS8, Line 77: DEFINE_int32
perhaps this should be _hidden unless we have some use case in mind where an admin would want to tune it?
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@389
PS8, Line 389: DCHECK_GT(timestamp_us, last_heartbeat_ts_.Load());
should this be _GE? it's possible (though unlikely) that we get called in rapid succession.
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@445
PS8, Line 445: if (initialized_.Load()) LOG(FATAL)
this can just be CHECK(!initialized_.Load())
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@552
PS8, Line 552: time_since_heartbeat
can you name this secs_since_heartbeat so the unit is clear?
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@916
PS8, Line 916: SetLastHeartbeatTimestamp(MonotonicMicros());
Given the argument to this method is always going to be MonotonicMicros(), what if the method was just called 'RefreshHeartbeatTimestamp()' or something and internally it called MonotonicMicros? Then, instead of the getter, you could have 'double TimeSinceHeartbeaat()' which calculates the subtraction. That would encapsulate the timing mechanism and units internal to the Subscriber class and only expose a minimal interface to callers
http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@1002
PS8, Line 1002: .size() == 0
empty
http://gerrit.cloudera.org:8080/#/c/11052/8/www/statestore_subscribers.tmpl
File www/statestore_subscribers.tmpl:
http://gerrit.cloudera.org:8080/#/c/11052/8/www/statestore_subscribers.tmpl@31
PS8, Line 31: Time since last heartbeat (seconds)
more concise: "Seconds since last heartbeat"
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 8
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 24 Aug 2018 16:15:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 6:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/11052/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11052/5//COMMIT_MSG@16
PS5, Line 16: Testing: Added an end to end test to verify the 'time_since_heartbeat'
: metric of a slow subscriber.
:
> sorry, one more thing I forgot to mention: can we add an automated e2e test
Done
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.h@444
PS5, Line 444:
> nit: technically this should be atomic, right? The "monitor" thread is acce
Done
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.h@687
PS5, Line 687: rapidjson::Document* document);
> the 'MainLoop' call seems to indicate that it would run until some exit fla
Done.
That comment was left around from quiet some time ago. The Statestore class no longer contains the exit_flag_ nor does it provide any API for graceful decommissioning. I have checked the current codebase and I can confirm that the statestore is never destructed. I have also added the destructor.
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@78
PS5, Line 78: s from a
> nit: typo
Done
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@315
PS5, Line 315: last_heartbeat_ts_(MonotonicMicros()) {
> is there any race here when a subscriber first registers before it sends a
Done
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@389
PS5, Line 389: DCHECK_GT(timestamp_us, last_heartbeat_ts_.Load());
> if these are wall times, it's possible for them to go backwards if the host
Done
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@542
PS5, Line 542:
> for the purposes of the user understanding this data, I'd think it would be
Done
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@984
PS5, Line 984: }
> You don't gain any real performance by declaring these outside of the loop,
Done
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@990
PS5, Line 990: int num_subscribers;
> maybe it would be clearer to not track this incrementally, but instead, jus
Done
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@992
PS5, Line 992: AGS_heartbeat_monitoring_
> nit: i think using 'const auto&' is just as good here
Done
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@998
PS5, Line 998: inactive_subscribers.push_back(subscriber.second->id());
> is it worth escalating this to WARN level if there are any inactive?
Done
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 16 Aug 2018 22:54:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.h@442
PS2, Line 442: of the most recently
> ... of the most recently logged heartbeat ...
Done
http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.h@446
PS2, Line 446: recent_logged_heartbeat_ts_
> This name sounds kind of misleading. To be more accurate, isn't it the " mo
Done
http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.cc@77
PS2, Line 77: statestore_heartbeat_log_frequency_seconds
> Just wondering if it's necessary to make this configurable. Are there scena
Agreed. As discussed on the issue's JIRA, it seems like we might need to tune this number based on the size/health of the cluster. What would you suggest here?
http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.cc@389
PS2, Line 389: DCHECK
> Use DCHECK_GT
Done
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 3
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 20:24:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
This patch collects the recent heartbeat timestamp of a subscriber
in 60 second intervals. It publishes this timsetamp to the
/subscribers page of the statestore and also adds it to the log.
Testing: Manually inspected the Web UI and statestore logs to
verify that the recent heartbeat timestamp for each subscriber is
updated periodically.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M www/statestore_subscribers.tmpl
3 files changed, 37 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11052/3
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 3
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.h@442
PS2, Line 442: of a recent heartbeat
... of the most recently logged heartbeat ...
http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.h@446
PS2, Line 446: recent_heartbeat_timestamp_
This name sounds kind of misleading. To be more accurate, isn't it the " most recently logged heartbeat timestamp" ? Since the actual default heartbeat frequency is much smaller than the default 'statestore_heartbeat_log_frequency_seconds'.
I'd suggest changing the name to 'recent_logged_heartbeat_ts_', and update the comments and related functions to reflect that too.
http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.cc@77
PS2, Line 77: statestore_heartbeat_log_frequency_seconds
Just wondering if it's necessary to make this configurable. Are there scenarios where users may want to change this, and if so, what's the benefit they get from doing so? If we don't have a strong reason to keep this flag, my vote is to get rid of it and keep 60 seconds (or some reasonable value) as a constant.
The more flags we add, the larger our test matrix needs to become, or we result in more untested code paths. So, I'd say we should add it only if it's absolutely necessary.
http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.cc@389
PS2, Line 389: DCHECK
Use DCHECK_GT
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 18:33:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 5:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/11052/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11052/4//COMMIT_MSG@9
PS4, Line 9: After this patch, the statestore keeps track of the last successful
: heartbeat timestamp which is exposed as a subscriber metric on the
: statestore debug page. Also adds a monitoring thread that
: periodically checks the last heartbeat timestamp for all the
: subscribers and l
> After this patch, the statestore keeps track of the last successful heartbe
Done
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@418
PS4, Line 418: ms);
> milliseconds or ms
Done
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@442
PS4, Line 442: /// The timestamp of the last successful heartbeat in milliseconds. A timestamp much
: /// older than the heartbeat frequency implies an unr
> The timestamp of the last successful heartbeat in milliseconds.
Done
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@444
PS4, Line 444: 4_t last_heartbe
> heartbeat frequency
Done
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@532
PS4, Line 532: :unique_ptr<Thread> heartbeat_monitoring_thread_;
> Thread that monitors the heartbeats of all subscribers.
Done
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@684
PS4, Line 684: rtbeat_monitoring_frequency_ms mi
> the heartbeats of all subscribers every
Done
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@686
PS4, Line 686:
> that
Done
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@686
PS4, Line 686:
: [[noreturn]] void MonitorSub
> subscriber's Id.
Done
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.cc@424
PS4, Line 424: DCHECK(metrics != NULL);
: metrics_ = metrics;
: num_subscribers_metric_ = metrics->AddGauge(STATESTORE_LIVE_SUBSCRIBERS, 0);
> move this to Init() and return status
Done
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.cc@1003
PS4, Line 1003: s,
> $0
Done
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.cc@1004
PS4, Line 1004:
> nit: add a space after comma
Done
http://gerrit.cloudera.org:8080/#/c/11052/4/www/statestore_subscribers.tmpl
File www/statestore_subscribers.tmpl:
http://gerrit.cloudera.org:8080/#/c/11052/4/www/statestore_subscribers.tmpl@31
PS4, Line 31: <th>Last heartbeat timestamp</th>
> nit: add " (ms)"
The timestamp is printed in the following format:
yyyy-mm-dd hh:mm:ss.ms
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 5
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Aug 2018 18:47:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 6:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/378/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 16 Aug 2018 23:27:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 4:
(12 comments)
looks good, mostly just nits
http://gerrit.cloudera.org:8080/#/c/11052/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11052/4//COMMIT_MSG@9
PS4, Line 9: This patch updates the last heartbeat timestamp of a subscriber
: everytime the subscriber heartbeats successfully. It also launches
: a monitoring thread which periodically checks the last heartbeat
: timestamp for all the subscribers has been updated and logs the
: slow subscribers.
After this patch, the statestore keeps track of the last successful heartbeat timestamp which is exposed as a subscriber metric on the statestore debug page. Also adds a monitoring thread that periodically checks the last heartbeat timestamp for all the subscribers and logs the IDs of those that have not been updated since the last periodic check.
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@418
PS4, Line 418: seconds
milliseconds or ms
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@442
PS4, Line 442: /// The timestamp of the last heartbeat in milliseconds. This timestamp is updated
: /// everytime the subscirber heartbeats successfully.
The timestamp of the last successful heartbeat in milliseconds.
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@444
PS4, Line 444: update frequency
heartbeat frequency
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@532
PS4, Line 532: Thread that monitors the subscriber's heartbeats.
Thread that monitors the heartbeats of all subscribers.
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@684
PS4, Line 684: the subscriber's heartbeats every
the heartbeats of all subscribers every
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@686
PS4, Line 686: the
that
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.h@686
PS4, Line 686: subscriber's
: /// information to the logs.
subscriber's Id.
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.cc@424
PS4, Line 424: Status status = Thread::Create("statestore-heartbeat", "heartbeat-monitoring-thread",
: &Statestore::MonitorSubscriberHeartbeat, this, &heartbeat_monitoring_thread_);
: if (!status.ok()) LOG(WARNING) << "Unable to start heartbeat monitoring thread.";
move this to Init() and return status
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.cc@1003
PS4, Line 1003: %s
$0
http://gerrit.cloudera.org:8080/#/c/11052/4/be/src/statestore/statestore.cc@1004
PS4, Line 1004: ,
nit: add a space after comma
http://gerrit.cloudera.org:8080/#/c/11052/4/www/statestore_subscribers.tmpl
File www/statestore_subscribers.tmpl:
http://gerrit.cloudera.org:8080/#/c/11052/4/www/statestore_subscribers.tmpl@31
PS4, Line 31: <th>Last heartbeat timestamp</th>
nit: add " (ms)"
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 4
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Aug 2018 01:38:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
This patch collects the recent heartbeat timestamp of a subscriber
in 60 second intervals. It publishes this timsetamp to the
/subscribers page of the statestore and also adds it to the log.
Testing: Manually inspected the Web UI and statestore logs to
verify that the recent heartbeat timestamp for each subscriber is
updated periodically.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M www/statestore_subscribers.tmpl
3 files changed, 37 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11052/2
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#13). ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add last heartbeat timestamp into Statestore metric
After this patch, the statestore keeps track of the time since the
last heartbeat for each subscriber. It is exposed as a subscriber
metric on the statestore debug page. It also adds a monitoring
thread that periodically checks the last heartbeat timestamp for
all subscribers and logs the IDs of those that have not been
updated since the last periodic check.
Testing: Added an end to end test to verify the 'sec_since_heartbeat'
metric of a slow subscriber.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/statestore/test_statestore.py
M www/statestore_subscribers.tmpl
4 files changed, 104 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11052/13
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 13
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 10:
lgtm, thanks for being patient with the requested changes!
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 10
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 28 Aug 2018 02:52:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 15: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 15
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 Aug 2018 22:05:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
Patch Set 3:
(1 comment)
> Patch Set 3:
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/11052/3/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/3/be/src/statestore/statestore.cc@913
PS3, Line 913: LOG(INFO) << "Recently logged heartbeat for subscriber " << subscriber->id()
: << " at " << ToStringFromUnix(current_timestamp);
: subscriber->SetRecentHeartbeatTimestamp(current_timestamp);
> just passing through here: I'm not entirely following the logic. It makes s
Makes sense. I will change it.
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 3
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 31 Jul 2018 16:44:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add last heartbeat timestamp into Statestore metric
This patch updates the last heartbeat timestamp of a subscriber
everytime the subscriber heartbeats successfully. It also launches
a monitoring thread which periodically checks the last heartbeat
timestamp for all the subscribers has been updated and logs the
slow subscribers.
Testing: Manually inspected the Web UI and statestore logs to
verify that the last heartbeat timestamp for each subscriber is
updated periodically.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M www/statestore_subscribers.tmpl
3 files changed, 69 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11052/4
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 4
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
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/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add last heartbeat timestamp into Statestore metric
After this patch, the statestore keeps track of the time since the
last heartbeat for each subscriber. It is exposed as a subscriber
metric on the statestore debug page. It also adds a monitoring
thread that periodically checks the last heartbeat timestamp for
all subscribers and logs the IDs of those that have not been
updated since the last periodic check.
Testing: Added an end to end test to verify the 'sec_since_heartbeat'
metric of a slow subscriber.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Reviewed-on: http://gerrit.cloudera.org:8080/11052
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/statestore/test_statestore.py
M www/statestore_subscribers.tmpl
4 files changed, 104 insertions(+), 3 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 16
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11052/6/tests/statestore/test_statestore.py
File tests/statestore/test_statestore.py:
http://gerrit.cloudera.org:8080/#/c/11052/6/tests/statestore/test_statestore.py@473
PS6, Line 473: "
flake8: E501 line too long (92 > 90 characters)
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 16 Aug 2018 22:55:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add last heartbeat timestamp into Statestore metric
After this patch, the statestore keeps track of the last successful
heartbeat timestamp which is exposed as a subscriber metric on the
statestore debug page. Also adds a monitoring thread that
periodically checks the last heartbeat timestamp for all the
subscribers and logs the IDs of those that have not been updated
since the last periodic check.
Testing: Manually inspected the Web UI and statestore logs to
verify that the last heartbeat timestamp for each subscriber is
updated periodically.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M www/statestore_subscribers.tmpl
3 files changed, 67 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11052/5
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 5
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
Patch Set 2:
Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/89/
Running initial code review checks. This is experimental - please report any issues to tarmstrong@cloudera.com or on this JIRA: IMPALA-7317
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jul 2018 19:18:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11052/1/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
http://gerrit.cloudera.org:8080/#/c/11052/1/be/src/statestore/statestore.h@417
PS1, Line 417: Set's
nit: typo
http://gerrit.cloudera.org:8080/#/c/11052/1/be/src/statestore/statestore.h@442
PS1, Line 442: This timestamp must be within the
: /// duration of statestore_heartbeat_log_frequency_seconds.
needs more explanation. how about instead:
"
Is updated every "statestore_heartbeat_log_frequency_seconds" seconds with the last heartbeat. A timestamp much older than the update frequency implies an unresponsive subscriber.
"
http://gerrit.cloudera.org:8080/#/c/11052/1/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/1/be/src/statestore/statestore.cc@390
PS1, Line 390: 60
FLAGS_statestore_heartbeat_log_frequency_seconds
http://gerrit.cloudera.org:8080/#/c/11052/1/be/src/statestore/statestore.cc@389
PS1, Line 389: // Ensure that the timestamp to be updated is greater than the current value stored.
: // We can't guarantee that the difference is exactly 60 seconds because we are storing
: // the heartbeat timestamp in seconds while the hearbeats are sent
maybe instead of the comment, a dcheck for the following should be self explanatory.
timestamp_seconds > (recent_heartbeat_timestamp_ + FLAGS_statestore_heartbeat_log_frequency_seconds)
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 1
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jul 2018 19:00:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/68/ : 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/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 1
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jul 2018 00:45:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 12:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/513/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 12
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 Aug 2018 01:07:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add last heartbeat timestamp into Statestore metric
After this patch, the statestore keeps track of the time since the
last heartbeat for each subscriber. It is exposed as a subscriber
metric on the statestore debug page. It also adds a monitoring
thread that periodically checks the last heartbeat timestamp for
all subscribers and logs the IDs of those that have not been
updated since the last periodic check.
Testing: Added an end to end test to verify the 'time_since_heartbeat'
metric of a slow subscriber.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/statestore/test_statestore.py
M www/statestore_subscribers.tmpl
4 files changed, 101 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11052/7
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 7
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 15:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3091/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 15
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 Aug 2018 18:55:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/178/ : 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/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 5
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Aug 2018 19:23:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 15: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 15
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 Aug 2018 18:55:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
Patch Set 3:
No Builds Executed
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 3
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 30 Jul 2018 22:34:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add last heartbeat timestamp into Statestore metric
After this patch, the statestore keeps track of the time since the
last heartbeat for each subscriber. It is exposed as a subscriber
metric on the statestore debug page. It also adds a monitoring
thread that periodically checks the last heartbeat timestamp for
all subscribers and logs the IDs of those that have not been
updated since the last periodic check.
Testing: Added an end to end test to verify the 'time_since_heartbeat'
metric of a slow subscriber.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/statestore/test_statestore.py
M www/statestore_subscribers.tmpl
4 files changed, 101 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11052/6
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11052/1/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
http://gerrit.cloudera.org:8080/#/c/11052/1/be/src/statestore/statestore.h@417
PS1, Line 417: Sets
> nit: typo
Done
http://gerrit.cloudera.org:8080/#/c/11052/1/be/src/statestore/statestore.h@442
PS1, Line 442: This timestamp is updated
: /// every "statestore_heartbeat_log_frequency_seconds" seco
> needs more explanation. how about instead:
Done
http://gerrit.cloudera.org:8080/#/c/11052/1/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/1/be/src/statestore/statestore.cc@390
PS1, Line 390: _h
> FLAGS_statestore_heartbeat_log_frequency_seconds
I have removed this comment and instead modified the DCHECK.
http://gerrit.cloudera.org:8080/#/c/11052/1/be/src/statestore/statestore.cc@389
PS1, Line 389: DCHECK(timestamp_seconds
: > (recent_heartbeat_timestamp_ + FLAGS_statestore_heartbeat_log_frequency_seconds));
: recent_heartbeat_timestamp_ = timestamp_seconds;
> maybe instead of the comment, a dcheck for the following should be self exp
Done
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jul 2018 19:18:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 13:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/519/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 13
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 Aug 2018 16:59:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 10:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/491/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 10
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 27 Aug 2018 18:26:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 9:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/478/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 9
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 24 Aug 2018 23:18:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/124/ : 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/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 4
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 01 Aug 2018 01:33:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 14: Code-Review+2
Carrying Todd's +2
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 14
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 Aug 2018 18:54:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11052/3/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/3/be/src/statestore/statestore.cc@913
PS3, Line 913: LOG(INFO) << "Recently logged heartbeat for subscriber " << subscriber->id()
: << " at " << ToStringFromUnix(current_timestamp);
: subscriber->SetRecentHeartbeatTimestamp(current_timestamp);
just passing through here: I'm not entirely following the logic. It makes sense that you don't want to issue a LOG() on every heartbeat, but for the purposes of updating the timestamp member of the subscriber, why not update it every time and reduce some complexity?
Also the wording of the log message might not make much sense outside the context of this patch. Maybe just "successfully heartbeat with subscriber X" since the timestamp itself is already going to be included in the log format?
Also once a minute times every host could end up being a lot of logs. Maybe it makes sense to do the logging from some separate thread which iterates over all subscribers, and just lists a summary like "99/100 hosts successfully heartbeat within the last 60 seconds. Slow hosts: foo.blah.com (83 seconds since heartbeat)"
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 3
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 30 Jul 2018 22:19:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add last heartbeat timestamp into Statestore metric
After this patch, the statestore keeps track of the time since the
last heartbeat for each subscriber. It is exposed as a subscriber
metric on the statestore debug page. It also adds a monitoring
thread that periodically checks the last heartbeat timestamp for
all subscribers and logs the IDs of those that have not been
updated since the last periodic check.
Testing: Added an end to end test to verify the 'sec_since_heartbeat'
metric of a slow subscriber.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/statestore/test_statestore.py
M www/statestore_subscribers.tmpl
4 files changed, 104 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11052/12
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 12
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 12:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11052/11/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
http://gerrit.cloudera.org:8080/#/c/11052/11/be/src/statestore/statestore.h@393
PS11, Line 393: tatic_
> nit: maybe use a static_cast
Done
http://gerrit.cloudera.org:8080/#/c/11052/11/be/src/statestore/statestore.h@466
PS11, Line 466: /// older than the heartbeat frequency implies an unresponsive subscriber.
> nit: AtomicInt64 last_heartbeat_ts_{0}
Done
http://gerrit.cloudera.org:8080/#/c/11052/11/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/11/be/src/statestore/statestore.cc@1001
PS11, Line 1001: for (const a
> nit: you can use SleepForMs instead and get rid of extra includes
Done
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 12
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 Aug 2018 00:32:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11052/3/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/3/be/src/statestore/statestore.cc@913
PS3, Line 913: else if (status.code() == TErrorCode::RPC_RECV_TIMEOUT) {
: // Add details to status to make it more useful, while preserving the stack
: status.AddDetail(Substitute(
> Makes sense. I will change it.
Done
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 4
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 01 Aug 2018 01:10:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/89/ : 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/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jul 2018 20:03:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add last heartbeat timestamp into Statestore metric
After this patch, the statestore keeps track of the time since the
last heartbeat for each subscriber. It is exposed as a subscriber
metric on the statestore debug page. It also adds a monitoring
thread that periodically checks the last heartbeat timestamp for
all subscribers and logs the IDs of those that have not been
updated since the last periodic check.
Testing: Added an end to end test to verify the 'sec_since_heartbeat'
metric of a slow subscriber.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/statestore/test_statestore.py
M www/statestore_subscribers.tmpl
4 files changed, 107 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11052/9
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 9
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 5:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.h@444
PS5, Line 444: int64_t last_heartbeat_ts_;
nit: technically this should be atomic, right? The "monitor" thread is accessing this variable only under 'subscribers_lock_', but this variable gets *set* from the heartbeat-sending thread which doesn't hold that lock, if I read the code correctly.
In practice it probably doesn't matter since it's just an int and x86 has a strong enough memory model, but I think it would be better to use a std::atomic<int64_t> here so that tools like TSAN don't complain.
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.h@687
PS5, Line 687: [[noreturn]] void MonitorSubscriberHeartbeat();
the 'MainLoop' call seems to indicate that it would run until some exit flag is set, but I don't see the exit flag. So, i'm not 100% sure: is it possible that a StateStore object ever destructs? If it did, I think this thread would keep on running and then crash as it accessed the freed memory of the StateStore object.
If we believe that StateStore can never destruct, can we add a destructor which does something like a LOG(FATAL) << "Cannot shut down StateStore object once started" in the case that it sees that Init() was called? That way if someone tries to destruct a StateStore we'll get an obvious crash instead of a tricky-to-debug one
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@78
PS5, Line 78: hearbeats
nit: typo
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@315
PS5, Line 315: last_heartbeat_ts_(0) {
is there any race here when a subscriber first registers before it sends a heartbeat where it will report as something like 48 years since heartbeat? Perhaps this should be set to the current time instead?
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@389
PS5, Line 389: DCHECK_GT(timestamp_ms, last_heartbeat_ts_);
if these are wall times, it's possible for them to go backwards if the host clock resets. Perhaps we should be using monotonic times here to avoid this issue?
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@542
PS5, Line 542: Value last_heartbeat_timestamp(
for the purposes of the user understanding this data, I'd think it would be more usable to output the number of seconds since the heartbeat, instead of the last heartbeat time. It's much easier to scan down a list of 100 subscribers and find the one with a big value, instead of trying to scan down a list of timestamps to find the one that is farthest in the past.
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@984
PS5, Line 984: int num_subscribers, num_active_subscribers;
You don't gain any real performance by declaring these outside of the loop, and I think you lose some clarity. For the ints, I don't think it makes a performance difference at all, since the compiler will just allocate stack slots for them either way, and there is no constructor/destructor. For the vector, in theory there might be some gain, but given this loop only runs once a minute I don't think saving potentially 1 tiny allocation matters.
Instead, could you move these down to inside the for loop so it's clear they don't cross from one loop iteration to the next? I think doing so will remove some lines of code and make it more obvious that only 'previous_timestamp' is carried from one iteration to the next.
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@990
PS5, Line 990: num_subscribers = subscribers_.size();
maybe it would be clearer to not track this incrementally, but instead, just before the log statement, do:
int num_active_subscrbers = num_subscribers - inactive_subscribers.size();
since it would reduce some extra branching and mental overhead
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@992
PS5, Line 992: SubscriberMap::value_type
nit: i think using 'const auto&' is just as good here
http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@998
PS5, Line 998: LOG(INFO) << num_active_subscribers << "/" << num_subscribers
is it worth escalating this to WARN level if there are any inactive?
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 5
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 08 Aug 2018 00:48:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add last heartbeat timestamp into Statestore metric
After this patch, the statestore keeps track of the time since the
last heartbeat for each subscriber. It is exposed as a subscriber
metric on the statestore debug page. It also adds a monitoring
thread that periodically checks the last heartbeat timestamp for
all subscribers and logs the IDs of those that have not been
updated since the last periodic check.
Testing: Added an end to end test to verify the 'sec_since_heartbeat'
metric of a slow subscriber.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/statestore/test_statestore.py
M www/statestore_subscribers.tmpl
4 files changed, 109 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11052/10
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 10
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 10
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 28 Aug 2018 02:51:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 14:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/522/ : 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/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 14
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 Aug 2018 18:46:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11052/6/tests/statestore/test_statestore.py
File tests/statestore/test_statestore.py:
http://gerrit.cloudera.org:8080/#/c/11052/6/tests/statestore/test_statestore.py@473
PS6, Line 473:
> flake8: E501 line too long (92 > 90 characters)
Done
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 7
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 17 Aug 2018 00:23:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#14). ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
IMPALA-6644: Add last heartbeat timestamp into Statestore metric
After this patch, the statestore keeps track of the time since the
last heartbeat for each subscriber. It is exposed as a subscriber
metric on the statestore debug page. It also adds a monitoring
thread that periodically checks the last heartbeat timestamp for
all subscribers and logs the IDs of those that have not been
updated since the last periodic check.
Testing: Added an end to end test to verify the 'sec_since_heartbeat'
metric of a slow subscriber.
Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M tests/statestore/test_statestore.py
M www/statestore_subscribers.tmpl
4 files changed, 104 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11052/14
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 14
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 7:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/383/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 7
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 17 Aug 2018 00:57:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11052/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11052/5//COMMIT_MSG@16
PS5, Line 16: Testing: Manually inspected the Web UI and statestore logs to
: verify that the last heartbeat timestamp for each subscriber is
: updated periodically.
sorry, one more thing I forgot to mention: can we add an automated e2e test to test_statestore.py that ensures that this data shows up and is reasonable? If you want to get fancy you could even kill -STOP one of the subscribers and make sure that it eventually shows up as inactive, but I'd be OK if you just do the basic thing of checking that some reasonable looking strings show up in the JSON.
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 5
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 08 Aug 2018 00:50:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 5
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 07 Aug 2018 22:18:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 13
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 Aug 2018 17:32:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11052/12/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/12/be/src/statestore/statestore.cc@78
PS12, Line 78: 6000
this went from 1 minute to 6 seconds here, was that intentional?
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 12
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 Aug 2018 00:44:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6644: Add last heartbeat timestamp into Statestore metric
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric
......................................................................
Patch Set 13:
(1 comment)
> Patch Set 12:
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/11052/12/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/11052/12/be/src/statestore/statestore.cc@78
PS12, Line 78: 6000
> this went from 1 minute to 6 seconds here, was that intentional?
It was a mistake. Fixed it.
--
To view, visit http://gerrit.cloudera.org:8080/11052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7
Gerrit-Change-Number: 11052
Gerrit-PatchSet: 13
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 Aug 2018 16:15:47 +0000
Gerrit-HasComments: Yes