Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
Great! That makes sense.
Thanks for the review @joshelser . I will be (squashing and) pushing this
to 4.x-HBase-1.4 branch and will back port / forward port this patch as well.
---
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
> It's just that some project using PQS wants to change configuration via
code, It wont be possible since HBaseConfiguration.create() will only read XML
on the class path
Downstream
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
> I'd just leave this as-is for now. I don't think it's "broken" how it is
right now (PQS doesn't try to set anything in the Configuration for the
PhoenixDriver to pick up).
It's just
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
> Is it fine for me to change
I'd just leave this as-is for now. I don't think it's "broken" how it is
right now (PQS doesn't try to set anything in the Configuration for the
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
> The setup of GlobalClientMetrics just... doesn't give us any way to do
this nicely
Yes, agreed on that one.
> I'm not sure what it takes to write a custom Factory (I think I did
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
> Am I missing something here?
Nope, it's me :). I didn't realize that the Factory was creating a new
`Configuration` every time. The setup of `GlobalClientMetrics` just... doesn't
give
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
> Isn't this as simple as changing QueryServer#main(String[]) to use
HBaseFactoryProvider.getConfigurationFactory().getConfiguration(); instead of
HBaseConfiguration.create()?
Yes I
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
@karanmehta93 but in PQS, we're controlling the Configuration. Isn't this
as simple as changing `QueryServer#main(String[])` to use
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
@joshelser I don't see an easy way of doing it, since we read the tag from
`metricTag = QueryServicesOptions.withDefaults().getClientMetricTag();`, which
internally uses `InstanceResolver`,
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
One final request, @karanmehta93 -- can you set
`phoenix.client.metrics.tag` in QueryServer.java proactively, please?
You have my +1 after that.
---
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
Thanks @xcangCRM and @joshelser for the review. I have addressed your
review comments and added tag support to differentiate between various types of
client when publishing metrics to JMX.
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
Just used JVisualVM to look at these metrics via the MBeans directly.
Worked just fine.
I think naming/convention when being collected from a distributed
environment needs to be thought
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
Thanks @joshelser will update the PR soon. We have immediate requirements
for HBase-1.x versions here. That is why I mentioned that at the start of PR
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
> it looks like there are some commons-configuration issues that will need
to be worked out for GlobalPhoenixMetricsTestSink.java
Just needs to be switched to the
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
Pulling this into `master`, it looks like there are some
commons-configuration issues that will need to be worked out for
GlobalPhoenixMetricsTestSink.java
---
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
> I think the only question pending in this PR is if we want JVMMetrics or
not, which are crucial from PQS POV, otherwise not really for a regular client.
Agreed. What about just making a
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
@joshelser I think the only question pending in this PR is if we want
JVMMetrics or not, which are crucial from PQS POV, otherwise not really for a
regular client. We can discuss what approach
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/315
> All these metrics are pushed to JMX and we monitor those via OpenTSDB
tcollectors.
Cool! I was just looking for a sanity check that you are seeing all of the
metrics you expect coming
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
@joshelser I was trying out this patch with an internal PQS version. All
these metrics are pushed to JMX and we monitor those via OpenTSDB tcollectors.
I haven't done any perf-test here, but I
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
Addressed your comments @joshelser, Please review!
---
Github user karanmehta93 commented on the issue:
https://github.com/apache/phoenix/pull/315
The patch might be tricky to port to other branches. Will also look into
that once the initial one gets in.
---
21 matches
Mail list logo
