You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2018/01/12 02:04:09 UTC
[Impala-ASF-CR] IMPALA-2397: Use atomics for metrics
Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9012
Change subject: IMPALA-2397: Use atomics for metrics
......................................................................
IMPALA-2397: Use atomics for metrics
This change removes the spinlock in IntGauge and IntCounter
and uses AtomicInt64 instead. As shown in IMPALA-2397, multiple
threads can be contending for the spinlocks of some global metrics
under concurrent queries.
This change also breaks up SimpleMetric into two subclasses:
- SimpleProperty:
- a value store for any primitive type (int,float,string etc).
- atomic read and write via value() and set_value() respectively.
- AtomicMetric:
- the basis of IntGauge and IntCounter. Support atomic increment
of the metric value via Increment() interface.
- atomic read and write via value() and set_value() respectively.
- only support int64_t type.
With this change, DoubleGauge is removed from the code base as the
existing use case doesn't make use of the Increment() interface so
replacing it with DoubleProperty is sufficient.
Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
---
M be/src/exec/external-data-source-executor.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/thrift-server.cc
M be/src/runtime/client-cache.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/mem-tracker-test.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/util/impalad-metrics.cc
M be/src/util/memory-metrics.cc
M be/src/util/memory-metrics.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.h
M be/src/util/thread.cc
M common/thrift/metrics.json
22 files changed, 267 insertions(+), 246 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/9012/1
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-2397: Use atomics for IntGauge and IntCounter
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for IntGauge and IntCounter
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Jan 2018 23:31:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2397: Use atomics for IntGauge and IntCounter
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9012
to look at the new patch set (#3).
Change subject: IMPALA-2397: Use atomics for IntGauge and IntCounter
......................................................................
IMPALA-2397: Use atomics for IntGauge and IntCounter
This change removes the spinlock in IntGauge and IntCounter
and uses AtomicInt64 instead. As shown in IMPALA-2397, multiple
threads can be contending for the spinlocks of some global metrics
under concurrent queries.
This change also breaks up SimpleMetric is renamed to ScalarMetric
and broken into two subclasses:
- LockedMetric:
- a value store for any primitive type (int,float,string etc).
- atomic read and write via GetValue() and SetValue() respectively.
- AtomicMetric:
- the basis of IntGauge and IntCounter. Support atomic increment
of the metric value via Increment() interface.
- atomic read and write via GetValue() and SetValue() respectively.
- only support int64_t type.
Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
---
M be/src/exec/external-data-source-executor.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/thrift-server.cc
M be/src/runtime/client-cache.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/io/scan-range.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/mem-tracker-test.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-state.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler.cc
M be/src/service/impala-server.cc
M be/src/service/session-expiry-test.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore.cc
M be/src/util/common-metrics.cc
M be/src/util/default-path-handlers.cc
M be/src/util/impalad-metrics.cc
M be/src/util/memory-metrics.cc
M be/src/util/memory-metrics.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.h
M be/src/util/thread.cc
M common/thrift/metrics.json
30 files changed, 353 insertions(+), 332 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/9012/3
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-2397: Use atomics for IntGauge and IntCounter
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for IntGauge and IntCounter
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Jan 2018 18:47:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2397: Use atomics for IntGauge and IntCounter
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/9012 )
Change subject: IMPALA-2397: Use atomics for IntGauge and IntCounter
......................................................................
IMPALA-2397: Use atomics for IntGauge and IntCounter
This change removes the spinlock in IntGauge and IntCounter
and uses AtomicInt64 instead. As shown in IMPALA-2397, multiple
threads can be contending for the spinlocks of some global metrics
under concurrent queries.
This change also breaks up SimpleMetric is renamed to ScalarMetric
and broken into two subclasses:
- LockedMetric:
- a value store for any primitive type (int,float,string etc).
- atomic read and write via GetValue() and SetValue() respectively.
- AtomicMetric:
- the basis of IntGauge and IntCounter. Support atomic increment
of the metric value via Increment() interface.
- atomic read and write via GetValue() and SetValue() respectively.
- only support int64_t type.
Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Reviewed-on: http://gerrit.cloudera.org:8080/9012
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exec/external-data-source-executor.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/thrift-server.cc
M be/src/runtime/client-cache.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/io/scan-range.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/mem-tracker-test.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-state.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler.cc
M be/src/service/impala-server.cc
M be/src/service/session-expiry-test.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore.cc
M be/src/util/common-metrics.cc
M be/src/util/default-path-handlers.cc
M be/src/util/impalad-metrics.cc
M be/src/util/memory-metrics.cc
M be/src/util/memory-metrics.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.h
M be/src/util/thread.cc
M common/thrift/metrics.json
30 files changed, 353 insertions(+), 332 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-2397: Use atomics for metrics
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for metrics
......................................................................
Patch Set 2: Code-Review+1
I don't have further comments, thanks for the inline replies.
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Jan 2018 22:10:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2397: Use atomics for metrics
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for metrics
......................................................................
Patch Set 2:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@128
PS1, Line 128: /// - 'property' which is a value store which can be read and written only
> Maybe mention that the metric kinds can serve as a hint for how management
Done
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@144
PS1, Line 144: /// Returns the current value. Thread-safe.
> I feel like we should rename to GetValue()/SetValue() since they're no long
Renamed to GetValue() / SetValue(). Keep them as pure virtual as their implementations can be different in derived classes but they share the implementation for the ToJson() interface.
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@147
PS1, Line 147: /// Sets the current value. Thread-safe.
> Does set_value() need to be a virtual function? I can't think of a case whe
Currently, there are only two derived classes for SimpleMetric and both of them implement this interface albeit differently. So, it seems to make sense to have it as virtual for now until we have more classes which inherit SimpleMetric but somehow doesn't implement SetValue().
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@215
PS1, Line 215: operty;
> nit: maybe rename to metric_kind_t to make it more clear that it's a type?
Done
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@225
PS1, Line 225: }
> The combination of value_/value()/CalculateValue() seems a bit too complica
Good idea. Renamed value() to GetValue().
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@232
PS1, Line 232: /// need to take care of synchronization among sources.
> Could this be a compile time check / assertion?
I believe the compiler will most likely fold this as a constant during compilation so it's a no-op for GAUGE.
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@252
PS1, Line 252: typedef class AtomicMetric<TMetricKind::COUNTER> IntCounter;
> I think it might be slightly easier to follow if we move these below the cl
Done
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@265
PS1, Line 265: int64_t sum = 0;
> The base class comment says that this happens atomically, but this implemen
The new code and the old code both don't hold locks of all gauges to prevent them from being written before reading them. So, although each read of the gauges is atomic by itself, there is no guarantee on atomicity between all the gauges.
The base class comments only means that it's up to the derived class to implement its own synchronization and the model above apparently seems good enough for SumGauge.
Note that value() and set_value() are racy by nature as the value can change right after the value is read. The atomicity guarantee has more to do with read-modify-write between multiple writers and atomic read of the value (i.e. no half written 64-bit value).
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@284
PS1, Line 284:
> nit: this might fit into a single line
Done
http://gerrit.cloudera.org:8080/#/c/9012/1/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/9012/1/common/thrift/metrics.json@886
PS1, Line 886: "kind": "PROPERTY",
> It seems like this should still be exposed as a gauge since it's a numeric
Unfortunately, if we set this to a GAUGE, we need to back it with a gauge in the code too. In which case, we may need to implement DoubleGauge.
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Jan 2018 19:18:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2397: Use atomics for IntGauge and IntCounter
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for IntGauge and IntCounter
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@147
PS1, Line 147: virtual void set_value(const T& value) = 0;
> What's the point of making it a virtual function if it doesn't need to be?
Done
http://gerrit.cloudera.org:8080/#/c/9012/1/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/9012/1/common/thrift/metrics.json@886
PS1, Line 886: "kind": "PROPERTY",
> I think SimpleProperty<double> implements all the functionality needed for
As discussed, renamed SimpleProperty to LockedMetrics and it's not confined to TMetricKind::PROPERTY. SimpleMetric is renamed to ScalarMetric.
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Jan 2018 01:47:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2397: Use atomics for metrics
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for metrics
......................................................................
IMPALA-2397: Use atomics for metrics
This change removes the spinlock in IntGauge and IntCounter
and uses AtomicInt64 instead. As shown in IMPALA-2397, multiple
threads can be contending for the spinlocks of some global metrics
under concurrent queries.
This change also breaks up SimpleMetric into two subclasses:
- SimpleProperty:
- a value store for any primitive type (int,float,string etc).
- atomic read and write via value() and set_value() respectively.
- AtomicMetric:
- the basis of IntGauge and IntCounter. Support atomic increment
of the metric value via Increment() interface.
- atomic read and write via value() and set_value() respectively.
- only support int64_t type.
With this change, DoubleGauge is removed from the code base as the
existing use case doesn't make use of the Increment() interface so
replacing it with DoubleProperty is sufficient.
Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
---
M be/src/exec/external-data-source-executor.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/thrift-server.cc
M be/src/runtime/client-cache.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/io/scan-range.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/mem-tracker-test.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-state.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler.cc
M be/src/service/impala-server.cc
M be/src/service/session-expiry-test.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/util/common-metrics.cc
M be/src/util/default-path-handlers.cc
M be/src/util/impalad-metrics.cc
M be/src/util/memory-metrics.cc
M be/src/util/memory-metrics.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.h
M be/src/util/thread.cc
M common/thrift/metrics.json
31 files changed, 353 insertions(+), 334 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/9012/2
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-2397: Use atomics for metrics
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for metrics
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@215
PS1, Line 215: metric_kind
nit: maybe rename to metric_kind_t to make it more clear that it's a type?
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@232
PS1, Line 232: DCHECK(metric_kind != TMetricKind::COUNTER || delta >= 0)
Could this be a compile time check / assertion?
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@252
PS1, Line 252: typedef class SimpleProperty<bool> BooleanProperty;
I think it might be slightly easier to follow if we move these below the class declaration of SimpleProperty.
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@265
PS1, Line 265: virtual int64_t CalculateValue() const override {
The base class comment says that this happens atomically, but this implementation looks like it doesn't. Is that intended?
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@284
PS1, Line 284: virtual int64_t CalculateValue() const override {
nit: this might fit into a single line
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Jan 2018 17:47:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2397: Use atomics for metrics
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for metrics
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@147
PS1, Line 147: virtual void set_value(const T& value) = 0;
> Currently, there are only two derived classes for SimpleMetric and both of
What's the point of making it a virtual function if it doesn't need to be? It seems better to just have a regular function on the derived classes - it's more efficient and easier to understand which function is being called.
I checked out this PS and made it non-virtual and it seems to compile and run fine.
http://gerrit.cloudera.org:8080/#/c/9012/1/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/9012/1/common/thrift/metrics.json@886
PS1, Line 886: "kind": "PROPERTY",
> Unfortunately, if we set this to a GAUGE, we need to back it with a gauge i
I think SimpleProperty<double> implements all the functionality needed for this one case, it would just need to declare itself as a TMetricKind::GAUGE.
I.e we could implement a skeleton DoubleGauge that only supports SetValue() and GetValue().
This isn't the end of the world, but it would be good if we could avoid the internal implementation details leaking out into our metadata about metrics.
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Jan 2018 20:14:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2397: Use atomics for IntGauge and IntCounter
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for IntGauge and IntCounter
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1749/
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Jan 2018 19:43:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2397: Use atomics for metrics
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for metrics
......................................................................
Patch Set 1:
(6 comments)
This should be a big improvement.
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/exec/external-data-source-executor.cc
File be/src/exec/external-data-source-executor.cc:
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/exec/external-data-source-executor.cc@79
PS1, Line 79: num_class_cache_hits_ = metrics->AddCounter(
Thanks for doing this cleanup.
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@128
PS1, Line 128: /// - 'property' which is a value store which can be read and written only
Maybe mention that the metric kinds can serve as a hint for how management software should present the metrics? I.e. it's not purely an internal concept.
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@144
PS1, Line 144: virtual T value() = 0;
I feel like we should rename to GetValue()/SetValue() since they're no longer just plain inline functions (although I guess that wasn't true even before because of the CalculateValue() bit).
If we make set_value() non-virtual like I suggest below, this comment may not apply there.
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@147
PS1, Line 147: virtual void set_value(const T& value) = 0;
Does set_value() need to be a virtual function? I can't think of a case where we'd call set_value() without knowing the subclass of the metric and I don't think every metric subclass necessarily needs an implementation of this, since some metrics are computed on demand.
http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@225
PS1, Line 225: virtual int64_t value() override { return CalculateValue(); }
The combination of value_/value()/CalculateValue() seems a bit too complicated. Couldn't subclasses just override value() instead of having the additional CalculateValue() indirection.
It's still a bit weird if a metric overrides CalculateValue() and never uses value_ but it seems like avoiding that would require refactoring the class hierarchy somehow, which probably isn't worth it.
http://gerrit.cloudera.org:8080/#/c/9012/1/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/9012/1/common/thrift/metrics.json@886
PS1, Line 886: "kind": "PROPERTY",
It seems like this should still be exposed as a gauge since it's a numeric value that can go up and down. Some management tools use this kind of metadata to decide how to display metrics.
I think the current SimpleProperty implementation is fine for this use case but we should allow it to be labelled as a GAUGE for external purposes.
--
To view, visit http://gerrit.cloudera.org:8080/9012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606
Gerrit-Change-Number: 9012
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Jan 2018 16:41:48 +0000
Gerrit-HasComments: Yes