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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 24 Aug 2018 22:40:51 +0000 Gerrit-HasComments: Yes
