PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi… #1431
Conversation
stoty
force-pushed
the
PHOENIX-6699
branch
3 times, most recently
from
956fc17
to
619eb11
Compare
|
@stoty - how does this affect (if at all) server-side metrics such as MetricsIndexerSource and GlobalIndexCheckerSource? |
Those should be added the HBase created MetricSystem structure (it's the happy path), but I'm going to do some more testing on a real cluster, and report back. |
phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java
Show resolved
Hide resolved
|
@joshelser @gjacoby126
|
There was a problem hiding this comment.
Could you write a quick unit test just to validate that this code doesn't throw a runtime exception? Wouldn't want to find out that this reflection gets broken as a result of some well-intended Hadoop change.
I acknowledge that the unit test may be very naive or even contrived -- it'd simply be good to make sure this reflection doesn't throw an exception (and cause some other breakage).
|
|
||
| @Test | ||
| //Check MetricsSystemImpl has a String "prefix" field in the Hadoop version we test with | ||
| public void testInternalMetricsApi() throws NoSuchFieldException, |
There was a problem hiding this comment.
Did you have something like this in mind, @elserj ?
|
|
||
| @Test | ||
| //Check that MetricsSystemImpl has a String "prefix" field in the Hadoop version we test with | ||
| public void testInternalMetricsField() throws NoSuchFieldException, |
| MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance(); | ||
| Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix"); | ||
| prefixField.setAccessible(true); | ||
| String oldValue = (String)prefixField.get(metrics); | ||
| prefixField.set(metrics, "dummy"); | ||
| prefixField.set(metrics, oldValue); | ||
| prefixField.setAccessible(false); |
There was a problem hiding this comment.
I was initially just thinking to make this MetricUtil.isDefaultMetricsInitialized() to avoid keeping this unit test aligned with the code in that method. I guess the likelihood of these drifting is low :)
There was a problem hiding this comment.
isDefaultMetricsInitialized() already has a catch-all section to avoid failing if the API changes, so it cannot really be tested.
…onServers