You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2022/04/28 05:25:18 UTC
[GitHub] [phoenix] stoty opened a new pull request, #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
stoty opened a new pull request, #1431:
URL: https://github.com/apache/phoenix/pull/1431
…onServers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
stoty commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r861982102
##########
phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java:
##########
@@ -38,4 +46,22 @@ public static MetricsStopWatch getMetricsStopWatch(boolean isRequestMetricsEnabl
return new MetricsStopWatch(true);
}
+ // We need to cover the case when JmxCacheBuster has just stopped the HBase metrics
+ // system, and not accidentally overwrite the DefaultMetricsSystem singleton.
+ // See PHOENIX-6699
+ public static boolean isDefaultMetricsInitialized() {
+ try {
+ MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix");
Review Comment:
Of course, this, like many Metrics stuff, is a huge hack, we shouldn't need to use reflection to check if DefaultMetricsSystem has been initialized already.
Also, we could just to see if we are and RS/Master process, and skip the initialization, but I don't know a way to do that.
Maybe you can suggest something ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] joshelser commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
joshelser commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r861966333
##########
phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java:
##########
@@ -38,4 +46,22 @@ public static MetricsStopWatch getMetricsStopWatch(boolean isRequestMetricsEnabl
return new MetricsStopWatch(true);
}
+ // We need to cover the case when JmxCacheBuster has just stopped the HBase metrics
+ // system, and not accidentally overwrite the DefaultMetricsSystem singleton.
+ // See PHOENIX-6699
+ public static boolean isDefaultMetricsInitialized() {
+ try {
+ MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix");
Review Comment:
`prefix` isn't `volatile` and is set from within the `synchronized` `init(String)` method. This would mean that we may be initialized and not yet be able to observe it from this separate thread.
That said, I don't know how likely this race condition would be to hit.
We could also check the `monitoring` boolean instead of `prefix`. This would mean that we already have atomic access to that variable. I am assuming that the synchronization on the methods in MetricsSystemImpl would not affect this reflection, meaning we would still have a (small) window where `stop()` is being called but not yet complete.
Maybe you have already thought about this :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
stoty commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r868157949
##########
phoenix-core/src/test/java/org/apache/phoenix/monitoring/MetricUtilTest.java:
##########
@@ -48,4 +52,17 @@ public void testGetMetricsStopWatchWithMetricsFalse() throws Exception {
LogLevel.OFF, WALL_CLOCK_TIME_MS);
assertFalse(metricsStopWatch.getMetricsEnabled());
}
+
+ @Test
+ //Check that MetricsSystemImpl has a String "prefix" field in the Hadoop version we test with
+ public void testInternalMetricsField() throws NoSuchFieldException,
Review Comment:
Renamed the test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
stoty commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r863505525
##########
phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java:
##########
@@ -38,4 +46,22 @@ public static MetricsStopWatch getMetricsStopWatch(boolean isRequestMetricsEnabl
return new MetricsStopWatch(true);
}
+ // We need to cover the case when JmxCacheBuster has just stopped the HBase metrics
+ // system, and not accidentally overwrite the DefaultMetricsSystem singleton.
+ // See PHOENIX-6699
+ public static boolean isDefaultMetricsInitialized() {
+ try {
+ MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix");
Review Comment:
> Could we somehow separate the Phoenix client-side-only metrics from the execution pathway of the Phoenix server-side metrics? I imagine this is no easier to do.
We're veering into territories that are beyond my knowledge of the metrics system.
AFAICT the Hadoop metrics system always uses the DefaultMetrics object, I haven't seen anything that points to using multiple MetricsSystem objects at the same time.
This is mostly fine, as we can add/remove metrics from this default MetricsSystem objects, and this doesn't cause any problems, and this means that we add the additional Phoenix metrics in the RS dynamically, and they can be accessed in a unified way.
Pre HBase 1.4, Phoenix didn't use the metrics2 framework for its client metrics, and it had to be accessed programmatically (no JMX). I don't think we want that.
The only problem comes from the fact that SOMETHING has to initialize a DefaultMetrics object on the Phoenix client side once before the Metrics System can be used, and
1. HBase (server side) and Phoenix (anywhere) attempt this
2. While DefaultMetrics is protected from re-initialization, the stop/start sequence preformed by JmxCacheBuster defeats this, hence this bug.
I have also tried to simply disable the DefaultMetrics initialization from Phoenix, but in that case the Metrics System wasn't initialized at all on the client side.
All in all, I think that my current solution is good enough, and I'm not convinced that expliring alternatives is worth the time (even though I asked for them from you)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] joshelser commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
joshelser commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r862109064
##########
phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java:
##########
@@ -38,4 +46,22 @@ public static MetricsStopWatch getMetricsStopWatch(boolean isRequestMetricsEnabl
return new MetricsStopWatch(true);
}
+ // We need to cover the case when JmxCacheBuster has just stopped the HBase metrics
+ // system, and not accidentally overwrite the DefaultMetricsSystem singleton.
+ // See PHOENIX-6699
+ public static boolean isDefaultMetricsInitialized() {
+ try {
+ MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix");
Review Comment:
> I don't think we have to worry about a race around prefix, AFAICT by the time the Phoenix classes are loaded, HBase has long initialized DefaultMetrics, and prefix has been set.
Ah, ok, I wondered if I was completely worrying about a non-issue.
> Of course, this, like many Metrics stuff, is a huge hack, we shouldn't need to use reflection to check if DefaultMetricsSystem has been initialized already.
Yeah, that's life with hadoop metrics2 :)
> Also, we could just to check if we are and RS/Master process, and skip the initialization, but I don't know a way to do that.
Hrm, I think we could do that (let me see what I can dig up). What about Geoffrey's point about the server-side metrics stuff we do have in Phoenix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] joshelser commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
joshelser commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r868160395
##########
phoenix-core/src/test/java/org/apache/phoenix/monitoring/MetricUtilTest.java:
##########
@@ -48,4 +52,17 @@ public void testGetMetricsStopWatchWithMetricsFalse() throws Exception {
LogLevel.OFF, WALL_CLOCK_TIME_MS);
assertFalse(metricsStopWatch.getMetricsEnabled());
}
+
+ @Test
+ //Check that MetricsSystemImpl has a String "prefix" field in the Hadoop version we test with
+ public void testInternalMetricsField() throws NoSuchFieldException,
+ SecurityException, IllegalArgumentException, IllegalAccessException {
+ 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);
Review 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 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
stoty commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r861979379
##########
phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java:
##########
@@ -38,4 +46,22 @@ public static MetricsStopWatch getMetricsStopWatch(boolean isRequestMetricsEnabl
return new MetricsStopWatch(true);
}
+ // We need to cover the case when JmxCacheBuster has just stopped the HBase metrics
+ // system, and not accidentally overwrite the DefaultMetricsSystem singleton.
+ // See PHOENIX-6699
+ public static boolean isDefaultMetricsInitialized() {
+ try {
+ MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix");
Review Comment:
_MetricsSystemImpl.monitoring_ is not useful for this purpose, as it is set to false when the metrics system is stopped.
This is why we can overwrite the stopped Default Metrics System:
https://github.com/apache/hadoop/blob/7bd7725532fd139d2e0e1662df7700f7ab95067a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSystemImpl.java#L156
_MetricsSystemImpl.prefix_ is only written by the init() method, it is not touched otherwise, so it can be used to check for the codition when the MetricsSystemImpl is initialized, but in a stopped state:
https://github.com/apache/hadoop/blob/7bd7725532fd139d2e0e1662df7700f7ab95067a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSystemImpl.java#L154
I don't think we have to worry about a race around prefix, AFAICT by the time the Phoenix classes are loaded, HBase has long initialized DefaultMetrics, and prefix has been set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] joshelser commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
joshelser commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r863210369
##########
phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java:
##########
@@ -38,4 +46,22 @@ public static MetricsStopWatch getMetricsStopWatch(boolean isRequestMetricsEnabl
return new MetricsStopWatch(true);
}
+ // We need to cover the case when JmxCacheBuster has just stopped the HBase metrics
+ // system, and not accidentally overwrite the DefaultMetricsSystem singleton.
+ // See PHOENIX-6699
+ public static boolean isDefaultMetricsInitialized() {
+ try {
+ MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix");
Review Comment:
> we could just to check if we are and RS/Master process, and skip the initialization, but I don't know a way to do that.
I'm not coming up with any static state that you could look at to try to determine if you're in the context of a RS or Master, shy of _hoping_ that the classpath client side doesnt' have the `HRegionServer.class` in it (or similar).
The other thing I thought of was looking at `RpcServer.getCurrentCall()`. Normally, there is a corresponding user RPC which would result in this returning the current RPC. But, this method would return empty during a compaction (e.g. updating Phoenix stats) where there is no active HBase Call.
Could we somehow separate the Phoenix client-side-only metrics from the execution pathway of the Phoenix server-side metrics? I imagine this is no easier to do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
stoty commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r868156096
##########
phoenix-core/src/test/java/org/apache/phoenix/monitoring/MetricUtilTest.java:
##########
@@ -48,4 +52,17 @@ public void testGetMetricsStopWatchWithMetricsFalse() throws Exception {
LogLevel.OFF, WALL_CLOCK_TIME_MS);
assertFalse(metricsStopWatch.getMetricsEnabled());
}
+
+ @Test
+ //Check MetricsSystemImpl has a String "prefix" field in the Hadoop version we test with
+ public void testInternalMetricsApi() throws NoSuchFieldException,
Review Comment:
Did you have something like this in mind, @elserj ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty closed pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
stoty closed pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
URL: https://github.com/apache/phoenix/pull/1431
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
stoty commented on PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#issuecomment-1112886226
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#issuecomment-1112632063
@stoty - how does this affect (if at all) server-side metrics such as MetricsIndexerSource and GlobalIndexCheckerSource?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
stoty commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r862295756
##########
phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java:
##########
@@ -38,4 +46,22 @@ public static MetricsStopWatch getMetricsStopWatch(boolean isRequestMetricsEnabl
return new MetricsStopWatch(true);
}
+ // We need to cover the case when JmxCacheBuster has just stopped the HBase metrics
+ // system, and not accidentally overwrite the DefaultMetricsSystem singleton.
+ // See PHOENIX-6699
+ public static boolean isDefaultMetricsInitialized() {
+ try {
+ MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix");
Review Comment:
The server side Phoenix metrics should be added to the Metrics System initialized by HBase, and there should be no change with this patch.
I haven't got around to confirming that on a real cluster yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
stoty commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r861982102
##########
phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java:
##########
@@ -38,4 +46,22 @@ public static MetricsStopWatch getMetricsStopWatch(boolean isRequestMetricsEnabl
return new MetricsStopWatch(true);
}
+ // We need to cover the case when JmxCacheBuster has just stopped the HBase metrics
+ // system, and not accidentally overwrite the DefaultMetricsSystem singleton.
+ // See PHOENIX-6699
+ public static boolean isDefaultMetricsInitialized() {
+ try {
+ MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix");
Review Comment:
Of course, this, like many Metrics stuff, is a huge hack, we shouldn't need to use reflection to check if DefaultMetricsSystem has been initialized already.
Also, we could just to check if we are and RS/Master process, and skip the initialization, but I don't know a way to do that.
Maybe you can suggest something ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
stoty commented on PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#issuecomment-1115151165
@joshelser @gjacoby126
Now I was able to test a real cluster with this patch.
The phoenix metrics are added to the HBase metrics just fine, no change.
`{
"name": "Hadoop:service=HBase,name=RegionServer,sub=PhoenixIndexer",
"modelerType": "RegionServer,sub=PhoenixIndexer",
"tag.Context": "phoenix",
"tag.Hostname": "stoty-3.stoty.root.hwx.site",
"IndexPrepareTime_num_ops": 2,
"IndexPrepareTime_min": 0,
"IndexPrepareTime_max": 0,`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
stoty commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r868191956
##########
phoenix-core/src/test/java/org/apache/phoenix/monitoring/MetricUtilTest.java:
##########
@@ -48,4 +52,17 @@ public void testGetMetricsStopWatchWithMetricsFalse() throws Exception {
LogLevel.OFF, WALL_CLOCK_TIME_MS);
assertFalse(metricsStopWatch.getMetricsEnabled());
}
+
+ @Test
+ //Check that MetricsSystemImpl has a String "prefix" field in the Hadoop version we test with
+ public void testInternalMetricsField() throws NoSuchFieldException,
+ SecurityException, IllegalArgumentException, IllegalAccessException {
+ 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);
Review Comment:
isDefaultMetricsInitialized() already has a catch-all section to avoid failing if the API changes, so it cannot really be tested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] joshelser commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…
Posted by GitBox <gi...@apache.org>.
joshelser commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r868106600
##########
phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java:
##########
@@ -38,4 +46,22 @@ public static MetricsStopWatch getMetricsStopWatch(boolean isRequestMetricsEnabl
return new MetricsStopWatch(true);
}
+ // We need to cover the case when JmxCacheBuster has just stopped the HBase metrics
+ // system, and not accidentally overwrite the DefaultMetricsSystem singleton.
+ // See PHOENIX-6699
+ public static boolean isDefaultMetricsInitialized() {
+ try {
+ MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix");
Review Comment:
> All in all, I think that my current solution is good enough, and I'm not convinced that expliring alternatives is worth the time (even though I asked for them from you)
Yeah, I think your assessment is fair. We carry lots of baggage as a result of Hadoop metrics2. This isn't the worst thing that I've seen done :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org