[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
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 users would instantiate QueryServer directly, not call `main()`. Still, the bigger issue is the ConfigurationFactory being ineffective. ---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
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 PhoenixDriver to pick up). ---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
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 us any way to do this nicely :( I'm not sure what it takes to write a custom Factory (I think I did it once in an IT), but would be OK if you defer this. After you commit this, please update the website with these necessary changes (e.g. at least tell people that PQS exports metrics). ---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
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 `HBaseFactoryProvider.getConfigurationFactory().getConfiguration();` instead of `HBaseConfiguration.create()`? ---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
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] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207051474
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java
---
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from
hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's
DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+private static final Log LOG =
LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+private static GlobalMetricRegistriesAdapter INSTANCE;
+
+private GlobalMetricRegistriesAdapter() {
+DefaultMetricsSystem.initialize("HBase");
+JvmMetrics.initSingleton("HBase", "");
--- End diff --
I don't have a strong opinion on how you expose this to let PQS say "I am
PQS" (e.g. whether CSV, list, whatever).
But yeah, modifying `snapshotAllMetrics` to include a Hadoop Metrics2
MetricsTag should do the trick.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r207003788
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java
---
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from
hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's
DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+private static final Log LOG =
LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+private static GlobalMetricRegistriesAdapter INSTANCE;
+
+private GlobalMetricRegistriesAdapter() {
+DefaultMetricsSystem.initialize("HBase");
+JvmMetrics.initSingleton("HBase", "");
--- End diff --
> I am sorry, I didn't you on this one. Could you elaborate?
You're using openTSDB to collect all of this data and your use case is to
"understand how PQS runs". To do this, you will need to know what hosts that
PQS is running on to just look at PQS metrics. I was suggesting that having a
unique name (or a tag) would make an operator's life much more simple -- it
would be clear (and presumably easier to filter on) to find just PQS metrics.
For example, the difference of issuing a filter "host in pqs_server1,
pqs_server2, pqs_server3" as opposed to just "tag=PQS", or similar.
Now that I'm thinking about this, leaving this as {{Phoenix,sub=CLIENT}}
may be OK and you can instead just configure the push of metrics data to Hadoop
Metrics2 to include a "tag" that indicates this is from PQS. That would make me
happy.
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
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 about more though. ---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206974865
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java
---
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from
hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's
DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+private static final Log LOG =
LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+private static GlobalMetricRegistriesAdapter INSTANCE;
+
+private GlobalMetricRegistriesAdapter() {
+DefaultMetricsSystem.initialize("HBase");
+JvmMetrics.initSingleton("HBase", "");
--- End diff --
> why do we need to differentiate
As an operator, I'm going to expect much different characteristics coming
from client applications than I am from PQS. Since PQS is a SPOF for
(potentially) many users, I would expect a higher level of regularity from PQS
than I would a client's app.
> We should get that info from the application JVM itself right
I'm sure there's something you can apply to find a PQS metric compared to
other clients, but I am just thinking that it would be good to make this as
easy as possible. I think watching PQS metrics would be a common use-case, not
the exception.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206971126
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java
---
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from
hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's
DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+private static final Log LOG =
LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+private static GlobalMetricRegistriesAdapter INSTANCE;
+
+private GlobalMetricRegistriesAdapter() {
+DefaultMetricsSystem.initialize("HBase");
+JvmMetrics.initSingleton("HBase", "");
--- End diff --
How will you differentiate this "client" (PQS) from other clients?
---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
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 `org.apache.commons.configuration2` package. Also, GlobalPhoenixMetricsTestSink has a lot of unused imports. Would be good to make a pass over your changes for those. ---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
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] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
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 new configuration key in QueryServices/QueryServicesOptions for including JVM metrics in client-metrics which defaults to `false`? Then, have PQS always set this to be `true` via code. Going to pull this down right now and look at it too out of curiosity. ---
[GitHub] phoenix issue #315: PHOENIX-3655 Global Phoenix Client Metrics for PQS
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 out of PQS and that they are "useful". No need to perform any perf testing for me. ---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206638567
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java
---
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from
hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's
DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+private static final Log LOG =
LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+private static GlobalMetricRegistriesAdapter INSTANCE;
+
+private GlobalMetricRegistriesAdapter() {
+DefaultMetricsSystem.initialize("HBase");
+JvmMetrics.initSingleton("HBase", "");
--- End diff --
Oh, right. I keep getting confused over that.
JVM metrics being exported from PQS makes total sense. +1 to that
I am now wondering: should we be exporting Phoenix metrics out of PQS under
"Client" or make some specific "sub=PQS" or similar for it?
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206597366
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +230,45 @@ private static void resetGlobalMetrics() {
}
}
+// Phoenix Client Metrics are transported via Hadoop-metrics2 sink
+// The test sink is defined at GlobalPhoenixMetricsTestSink
+// Configuration for Hadoop-metrics2 comes from
hadoop-metrics2.properties file located in test/resources
+private boolean verifyMetricsFromSink() throws InterruptedException {
+Map expectedMetrics = new HashMap<>();
+for (GlobalMetric m :
PhoenixRuntime.getGlobalPhoenixClientMetrics()) {
+expectedMetrics.put(m.getMetricType().name(), m.getTotalSum());
+}
+
+for (int i = 0; i < MAX_RETRIES; i++) {
+LOG.info("Verifying Global Metrics from Hadoop Sink, Retry: "
+ (i + 1));
+if (verifyMetricsFromSinkOnce(expectedMetrics)) {
+LOG.info("Values from Hadoop Metrics Sink match actual
values");
+return true;
+}
+Thread.sleep(1000);
+}
+return false;
+}
+
+private boolean verifyMetricsFromSinkOnce(Map
expectedMetrics) {
+synchronized (GlobalPhoenixMetricsTestSink.lock) {
+for (AbstractMetric metric :
GlobalPhoenixMetricsTestSink.metrics) {
+if (expectedMetrics.containsKey(metric.name())) {
--- End diff --
I thought there was an issue with unboxing the value from
`expectedMetrics.get(metric.name())`.
I think it would be cleaner to do:
```
Long value = expectedMetrics.get(metric.name());
if (value != null) {
long expectedValue = value.longValue();
...
```
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206598366
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java
---
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from
hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's
DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+private static final Log LOG =
LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+private static GlobalMetricRegistriesAdapter INSTANCE;
+
+private GlobalMetricRegistriesAdapter() {
+DefaultMetricsSystem.initialize("HBase");
+JvmMetrics.initSingleton("HBase", "");
--- End diff --
I think collecting JVM metrics from client applications is heavy-handed.
I'd suggest you drop this.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206598488
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +230,45 @@ private static void resetGlobalMetrics() {
}
}
+// Phoenix Client Metrics are transported via Hadoop-metrics2 sink
+// The test sink is defined at GlobalPhoenixMetricsTestSink
+// Configuration for Hadoop-metrics2 comes from
hadoop-metrics2.properties file located in test/resources
--- End diff --
Nice!
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206598832
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/monitoring/GlobalPhoenixMetricsTestSink.java
---
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.monitoring;
+
+import org.apache.commons.configuration.SubsetConfiguration;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.metrics2.AbstractMetric;
+import org.apache.hadoop.metrics2.MetricsRecord;
+import org.apache.hadoop.metrics2.MetricsSink;
+import org.apache.phoenix.util.PhoenixRuntime;
+
+import java.util.Map;
+
+import static junit.framework.TestCase.assertTrue;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+public class GlobalPhoenixMetricsTestSink implements MetricsSink {
+
+public static final String PHOENIX_METRICS_RECORD_NAME = "PHOENIX";
+static Object lock = new Object();
--- End diff --
Can you leave a comment here about the importance of this lock? e.g. that
the caller should hold it to read `metrics`.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206595985
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +230,45 @@ private static void resetGlobalMetrics() {
}
}
+// Phoenix Client Metrics are transported via Hadoop-metrics2 sink
+// The test sink is defined at GlobalPhoenixMetricsTestSink
+// Configuration for Hadoop-metrics2 comes from
hadoop-metrics2.properties file located in test/resources
+private boolean verifyMetricsFromSink() throws InterruptedException {
+Map expectedMetrics = new HashMap<>();
+for (GlobalMetric m :
PhoenixRuntime.getGlobalPhoenixClientMetrics()) {
+expectedMetrics.put(m.getMetricType().name(), m.getTotalSum());
+}
+
+for (int i = 0; i < MAX_RETRIES; i++) {
+LOG.info("Verifying Global Metrics from Hadoop Sink, Retry: "
+ (i + 1));
+if (verifyMetricsFromSinkOnce(expectedMetrics)) {
+LOG.info("Values from Hadoop Metrics Sink match actual
values");
+return true;
+}
+Thread.sleep(1000);
--- End diff --
Change this to:
```
try {
Thread.sleep(1000);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
return false;
}
```
Then, remove the `throws InterruptedException`.
We should be catching, re-setting the interrupted state and just returning
ASAP. Likely, this code never gets invoked in a unit-test context, but good
practice.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206598160
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java
---
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.monitoring;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Gauge;
+import org.apache.hadoop.hbase.metrics.Histogram;
+import org.apache.hadoop.hbase.metrics.Meter;
+import org.apache.hadoop.hbase.metrics.Metric;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.metrics.Timer;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableHistogram;
+import org.apache.hadoop.metrics2.source.JvmMetrics;
+
+/**
+ * Contents mostly copied from GlobalMetricRegistriesAdapter class from
hbase-hadoop2-compat
+ * The adapter attaches HBase's MetricRegistry to Hadoop's
DefaultMetricsSystem
+ * Doesn't handle dynamic attach/detach of registries
+ */
+public class GlobalMetricRegistriesAdapter {
+
+private static final Log LOG =
LogFactory.getLog(GlobalMetricRegistriesAdapter.class);
+private static GlobalMetricRegistriesAdapter INSTANCE;
+
+private GlobalMetricRegistriesAdapter() {
+DefaultMetricsSystem.initialize("HBase");
--- End diff --
s/HBase/Phoenix/
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206259317
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +219,39 @@ private static void resetGlobalMetrics() {
}
}
+private boolean verifyMetricsFromSink() throws InterruptedException {
+Map expectedMetrics = new HashMap<>();
+for (GlobalMetric m :
PhoenixRuntime.getGlobalPhoenixClientMetrics()) {
+expectedMetrics.put(m.getMetricType().name(), m.getTotalSum());
+}
+
+for (int i = 0; i < MAX_RETRIES; i++) {
+LOG.info("Verifying Global Metrics from Hadoop Sink, Retry: "
+ (i + 1));
+if (verifyMetricsFromSinkOnce(expectedMetrics)) {
+LOG.info("Values from Hadoop Metrics Sink match actual
values");
+return true;
+}
+Thread.sleep(1000);
+}
+return false;
+}
+
+private boolean verifyMetricsFromSinkOnce(Map
expectedMetrics) {
+synchronized (GlobalPhoenixMetricsTestSink.lock) {
+for (AbstractMetric metric :
GlobalPhoenixMetricsTestSink.metrics) {
+if (expectedMetrics.containsKey(metric.name())) {
--- End diff --
> One approach can be to remove each entry from the map and check if the
size is 0 at the end. How does that sound?
That would work too!
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r206259129
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalClientMetrics.java
---
@@ -108,40 +114,88 @@
GLOBAL_HBASE_COUNT_ROWS_SCANNED(COUNT_ROWS_SCANNED),
GLOBAL_HBASE_COUNT_ROWS_FILTERED(COUNT_ROWS_FILTERED);
-
+private static final Logger LOG =
LoggerFactory.getLogger(GlobalClientMetrics.class);
private static final boolean isGlobalMetricsEnabled =
QueryServicesOptions.withDefaults().isGlobalMetricsEnabled();
+private MetricType metricType;
private GlobalMetric metric;
-public void update(long value) {
+static {
+initPhoenixGlobalClientMetrics();
if (isGlobalMetricsEnabled) {
-metric.change(value);
+MetricRegistry metricRegistry = createMetricRegistry();
+registerPhoenixMetricsToRegistry(metricRegistry);
+
GlobalMetricRegistriesAdapter.getInstance().registerMetricRegistry(metricRegistry);
+}
+}
+
+private static void initPhoenixGlobalClientMetrics() {
+for (GlobalClientMetrics globalMetric :
GlobalClientMetrics.values()) {
+globalMetric.metric = isGlobalMetricsEnabled ?
+new GlobalMetricImpl(globalMetric.metricType) : new
NoOpGlobalMetricImpl();
+}
+}
+
+private static void registerPhoenixMetricsToRegistry(MetricRegistry
metricRegistry) {
+for (GlobalClientMetrics globalMetric :
GlobalClientMetrics.values()) {
+metricRegistry.register(globalMetric.metricType.columnName(),
+new PhoenixGlobalMetricGauge(globalMetric.metric));
+}
+}
+
+private static MetricRegistry createMetricRegistry() {
+LOG.info("Creating Metric Registry for Phoenix Global Metrics");
+MetricRegistryInfo registryInfo = new
MetricRegistryInfo("PHOENIX", "Phoenix Global Metrics",
--- End diff --
> Global here refers that it is an aggregation across all clients (or all
Phoenix Connections).
In my mind, if you show me "Phoenix Client Metrics", I would naturally
assume that they are for all clients that have metrics enabled (as opposed to
breakouts by individual clients). As such, the word "Global" to me is just
filler, but maybe that's just my interpretation of it.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/315#discussion_r206259200 --- Diff: phoenix-core/src/test/resources/hadoop-metrics2.properties --- @@ -32,10 +32,9 @@ #[prefix].[source|sink].[instance].[options] # See javadoc of package-info.java for org.apache.hadoop.metrics2 for detail - -# Don't attempt to start jmx mbeans for all sources. -# For right now, all metrics are exported to a phoenix table -*.source.start_mbeans=false +hbase.source.start_mbeans=true +hbase.sink.sink0.class=org.apache.phoenix.monitoring.GlobalPhoenixMetricsTestSink --- End diff -- Haha, perfect :) ---
[GitHub] phoenix issue #318: Fixed Spelling.
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/318 @jimmycasey can you create a corresponding Jira issue under the PHOENIX project using https://issues.apache.org/jira/secure/CreateIssue.jspa, please? We do our project mgmt/release tracking on Jira. ---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/315#discussion_r205884608 --- Diff: phoenix-core/src/test/resources/hadoop-metrics2.properties --- @@ -32,10 +32,9 @@ #[prefix].[source|sink].[instance].[options] # See javadoc of package-info.java for org.apache.hadoop.metrics2 for detail - -# Don't attempt to start jmx mbeans for all sources. -# For right now, all metrics are exported to a phoenix table -*.source.start_mbeans=false +hbase.source.start_mbeans=true +hbase.sink.sink0.class=org.apache.phoenix.monitoring.GlobalPhoenixMetricsTestSink --- End diff -- Can you leave a comment in the test that this configuration is here? Otherwise, might seem like voodoo ---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r205884335
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +219,39 @@ private static void resetGlobalMetrics() {
}
}
+private boolean verifyMetricsFromSink() throws InterruptedException {
+Map expectedMetrics = new HashMap<>();
+for (GlobalMetric m :
PhoenixRuntime.getGlobalPhoenixClientMetrics()) {
+expectedMetrics.put(m.getMetricType().name(), m.getTotalSum());
+}
+
+for (int i = 0; i < MAX_RETRIES; i++) {
+LOG.info("Verifying Global Metrics from Hadoop Sink, Retry: "
+ (i + 1));
+if (verifyMetricsFromSinkOnce(expectedMetrics)) {
+LOG.info("Values from Hadoop Metrics Sink match actual
values");
+return true;
+}
+Thread.sleep(1000);
+}
+return false;
+}
+
+private boolean verifyMetricsFromSinkOnce(Map
expectedMetrics) {
+synchronized (GlobalPhoenixMetricsTestSink.lock) {
+for (AbstractMetric metric :
GlobalPhoenixMetricsTestSink.metrics) {
+if (expectedMetrics.containsKey(metric.name())) {
--- End diff --
This check doesn't catch the case where you have `expectedMetrics` but
`GlobalPhoenixMetricsTestSink.metrics` is empty. I'd switch this around to
iterate over your expectations, ensuring that they all exist.
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r205884792
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalClientMetrics.java
---
@@ -108,40 +114,88 @@
GLOBAL_HBASE_COUNT_ROWS_SCANNED(COUNT_ROWS_SCANNED),
GLOBAL_HBASE_COUNT_ROWS_FILTERED(COUNT_ROWS_FILTERED);
-
+private static final Logger LOG =
LoggerFactory.getLogger(GlobalClientMetrics.class);
private static final boolean isGlobalMetricsEnabled =
QueryServicesOptions.withDefaults().isGlobalMetricsEnabled();
+private MetricType metricType;
private GlobalMetric metric;
-public void update(long value) {
+static {
+initPhoenixGlobalClientMetrics();
if (isGlobalMetricsEnabled) {
-metric.change(value);
+MetricRegistry metricRegistry = createMetricRegistry();
+registerPhoenixMetricsToRegistry(metricRegistry);
+
GlobalMetricRegistriesAdapter.getInstance().registerMetricRegistry(metricRegistry);
+}
+}
+
+private static void initPhoenixGlobalClientMetrics() {
+for (GlobalClientMetrics globalMetric :
GlobalClientMetrics.values()) {
+globalMetric.metric = isGlobalMetricsEnabled ?
+new GlobalMetricImpl(globalMetric.metricType) : new
NoOpGlobalMetricImpl();
+}
+}
+
+private static void registerPhoenixMetricsToRegistry(MetricRegistry
metricRegistry) {
+for (GlobalClientMetrics globalMetric :
GlobalClientMetrics.values()) {
+metricRegistry.register(globalMetric.metricType.columnName(),
+new PhoenixGlobalMetricGauge(globalMetric.metric));
+}
+}
+
+private static MetricRegistry createMetricRegistry() {
+LOG.info("Creating Metric Registry for Phoenix Global Metrics");
+MetricRegistryInfo registryInfo = new
MetricRegistryInfo("PHOENIX", "Phoenix Global Metrics",
--- End diff --
What does "Global" mean in this context? Is "Phoenix Client Metrics" more
reasonable? Maybe "Phoenix,sub=CLIENT" down below?
---
[GitHub] phoenix pull request #315: PHOENIX-3655 Global Phoenix Client Metrics for PQ...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/315#discussion_r205884048
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
@@ -204,6 +219,39 @@ private static void resetGlobalMetrics() {
}
}
+private boolean verifyMetricsFromSink() throws InterruptedException {
+Map expectedMetrics = new HashMap<>();
+for (GlobalMetric m :
PhoenixRuntime.getGlobalPhoenixClientMetrics()) {
+expectedMetrics.put(m.getMetricType().name(), m.getTotalSum());
+}
+
+for (int i = 0; i < MAX_RETRIES; i++) {
+LOG.info("Verifying Global Metrics from Hadoop Sink, Retry: "
+ (i + 1));
+if (verifyMetricsFromSinkOnce(expectedMetrics)) {
+LOG.info("Values from Hadoop Metrics Sink match actual
values");
+return true;
+}
+Thread.sleep(1000);
+}
+return false;
+}
+
+private boolean verifyMetricsFromSinkOnce(Map
expectedMetrics) {
+synchronized (GlobalPhoenixMetricsTestSink.lock) {
--- End diff --
If it were me, I would have used a Queue or something to save off the
metrics, but there's nothing wrong with your approach here :)
---
[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/311#discussion_r204818876 --- Diff: bin/traceserver.py --- @@ -116,8 +116,10 @@ #" -Xdebug -Xrunjdwp:transport=dt_socket,address=5005,server=y,suspend=n " + \ #" -XX:+UnlockCommercialFeatures -XX:+FlightRecorder -XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true" + \ -java_cmd = '%(java)s $PHOENIX_OPTS ' + \ -'-cp ' + hbase_config_path + os.pathsep + phoenix_utils.phoenix_traceserver_jar + os.pathsep + phoenix_utils.phoenix_client_jar + \ +java_cmd = '%(java)s ' + \ --- End diff -- Actually, looks like queryserver.py doesn't include `$PHOENIX_OPTS` so let's just leave that to clients. `$PHOENIX_TRACESERVER_OPTS` is inlined, so you're good. Sorry for the confusion. ---
[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/311#discussion_r204795871 --- Diff: bin/traceserver.py --- @@ -116,8 +116,10 @@ #" -Xdebug -Xrunjdwp:transport=dt_socket,address=5005,server=y,suspend=n " + \ #" -XX:+UnlockCommercialFeatures -XX:+FlightRecorder -XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true" + \ -java_cmd = '%(java)s $PHOENIX_OPTS ' + \ -'-cp ' + hbase_config_path + os.pathsep + phoenix_utils.phoenix_traceserver_jar + os.pathsep + phoenix_utils.phoenix_client_jar + \ +java_cmd = '%(java)s ' + \ --- End diff -- The issue is that subprocess doesn't handle shell variable expansion "in the command". Subprocess has API to provide an environment to the process being run. `$PHOENIX_OPTS` and `$PHOENIX_TRACESERVER_OPTS` should be placed into the subprocess's environment, not just dropped. ---
[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/311#discussion_r203446592
--- Diff:
phoenix-tracing-webapp/src/main/java/org/apache/phoenix/tracingwebapp/http/TraceServlet.java
---
@@ -42,14 +45,21 @@
private static Connection con;
protected String DEFAULT_LIMIT = "25";
protected String DEFAULT_COUNTBY = "hostname";
+ protected String DEFAULT_DESCRIPTION = "description";
protected String LOGIC_AND = "AND";
protected String LOGIC_OR = "OR";
protected String TRACING_TABLE = "SYSTEM.TRACING_STATS";
-
+ @Override
+ public void init() {
+org.apache.hadoop.conf.Configuration conf =
HBaseConfiguration.create();
--- End diff --
Import `org.apache.hadoop.conf.Configuration` please.
---
[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/311#discussion_r203446437
--- Diff:
phoenix-tracing-webapp/src/main/java/org/apache/phoenix/tracingwebapp/http/Main.java
---
@@ -62,15 +63,18 @@ public int run(String[] arg0) throws Exception {
final String home = getConf().get(TRACE_SERVER_HTTP_JETTY_HOME_KEY,
DEFAULT_HTTP_HOME);
//setting up the embedded server
-ProtectionDomain domain = Main.class.getProtectionDomain();
-URL location = domain.getCodeSource().getLocation();
-String webappDirLocation = location.toString().split("target")[0]
+"src/main/webapp";
+String webappDirLocation = DEFAULT_WEBAPP_DIR_LOCATION;
--- End diff --
Suggest dropping `webappDirLocation` and just using
`DEFAULT_WEBAPP_DIR_LOCATION` since this is not configurable anyways.
---
[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/311#discussion_r203447995
--- Diff:
phoenix-tracing-webapp/src/main/java/org/apache/phoenix/tracingwebapp/http/TraceServlet.java
---
@@ -98,33 +108,36 @@ protected String getCount(String countby) {
if(countby == null) {
countby = DEFAULT_COUNTBY;
}
-// Throws exception if the column not present in the trace table.
-MetricInfo.getColumnName(countby.toLowerCase());
String sqlQuery = "SELECT "+countby+", COUNT(*) AS count FROM " +
TRACING_TABLE + " GROUP BY "+countby+" HAVING COUNT(*) > 1 ";
json = getResults(sqlQuery);
return json;
}
//search the trace over parent id or trace id
- protected String searchTrace(String parentId, String traceId,String
logic) {
+ protected String searchTrace(String parentId, String traceId, String
logic) {
+
String json = null;
String query = null;
// Check the parent Id, trace id type or long or not.
try {
+ if (parentId != null) {
Long.parseLong(parentId);
+ }
+ if (traceId != null) {
Long.parseLong(traceId);
+ }
} catch (NumberFormatException e) {
- throw new RuntimeException("The passed parentId/traceId is not
a number.", e);
+ throw new RuntimeException("The passed parentId/traceId is not a
number.", e);
}
-if(!logic.equals(LOGIC_AND) || !logic.equals(LOGIC_OR)) {
- throw new RuntimeException("Wrong logical operator passed to
the query. Only "+ LOGIC_AND+","+LOGIC_OR+" are allowed.") ;
+if (logic != null && (!logic.equals(LOGIC_AND) &&
!logic.equals(LOGIC_OR))) {
--- End diff --
Parenthesis around `(!logic.equals(LOGIC_AND) && !logic.equals(LOGIC_OR))`
are unnecessary, please drop them.
---
[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/311#discussion_r203445596 --- Diff: bin/traceserver.py --- @@ -116,8 +116,10 @@ #" -Xdebug -Xrunjdwp:transport=dt_socket,address=5005,server=y,suspend=n " + \ #" -XX:+UnlockCommercialFeatures -XX:+FlightRecorder -XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true" + \ -java_cmd = '%(java)s $PHOENIX_OPTS ' + \ -'-cp ' + hbase_config_path + os.pathsep + phoenix_utils.phoenix_traceserver_jar + os.pathsep + phoenix_utils.phoenix_client_jar + \ +java_cmd = '%(java)s ' + \ --- End diff -- Seems like `$PHOENIX_OPTS` was dropped. Was this not being interpolated correctly? Is this environment variable propagated into the traceserver java process? ---
[GitHub] phoenix issue #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/307 On the note about docker, trying to think about this, I feel like a docker environment that can spin up all of the necessary stuff to run both of the python tests as well as this new one will be best. Essentially, in the docker container, we have PQS up with all of the necessary environment stuff which will make all of our current tests (and any future test) that much easier to automate. I'm also happy to try to help with that. I know you've spent a bunch of time on this already @pu239ppy ---
[GitHub] phoenix issue #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/307 If there's a bright-side, it's something in my environment. Spinning up the JDBC thin client against this setup works: ``` $ PHOENIX_OPTS="$PHOENIX_OPTS -Dsun.security.krb5.debug=true -Djava.security.krb5.conf=/Users/jelser/projects/phoenix.git/phoenix-queryserver/target/test-data/8bc1abb8-79fa-4beb-aa56-fe3ae4edff64/kdc/1531499757782/krb5.conf " /usr/local/lib/phoenix-4.14.0-HBase-1.4/bin/sqlline-thin.py http://localhost:60358 -a SPNEGO ``` ---
[GitHub] phoenix issue #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/307
Turning back on `KRB5_TRACE`...
```
DEBUG:phoenixdb.avatica.client:POST http://localhost:60358/
'\n?org.apache.calcite.avatica.proto.Requests$OpenConnectionRequest\x12&\n$386e3317-e23e-4a0e-9fc6-2efaa546ffc4'
{'content-type': 'application/x-google-protobuf'}
DEBUG:urllib3.connectionpool:Starting new HTTP connection (1):
localhost:60358
send: 'POST / HTTP/1.1\r\nHost: localhost:60358\r\nConnection:
keep-alive\r\nAccept-Encoding: gzip, deflate\r\nAccept: */*\r\nUser-Agent:
python-requests/2.19.1\r\ncontent-type:
application/x-google-protobuf\r\nContent-Length:
105\r\n\r\n\n?org.apache.calcite.avatica.proto.Requests$OpenConnectionRequest\x12&\n$386e3317-e23e-4a0e-9fc6-2efaa546ffc4'
reply: 'HTTP/1.1 401 Unauthorized\r\n'
header: Date: Fri, 13 Jul 2018 17:23:46 GMT
header: WWW-Authenticate: Negotiate
header: Cache-Control: must-revalidate,no-cache,no-store
header: Content-Type: text/html; charset=ISO-8859-1
header: Content-Length: 281
header: Server: Jetty(9.2.19.v20160908)
DEBUG:urllib3.connectionpool:http://localhost:60358 "POST / HTTP/1.1" 401
281
DEBUG:requests_kerberos.kerberos_:handle_401(): Handling: 401
[28575] 1531502626.856661: ccselect module realm chose cache
FILE:/tmp/krb5cc_502 with client principal us...@example.com for server
principal HTTP/localh...@example.com
[28575] 1531502626.856662: Getting credentials us...@example.com ->
HTTP/localhost@ using ccache FILE:/tmp/krb5cc_502
[28575] 1531502626.856663: Retrieving us...@example.com -> HTTP/localhost@
from FILE:/tmp/krb5cc_502 with result: -1765328243/Matching credential not
found (filename: /tmp/krb5cc_502)
[28575] 1531502626.856664: Retrying us...@example.com ->
HTTP/localh...@example.com with result: -1765328243/Matching credential not
found (filename: /tmp/krb5cc_502)
[28575] 1531502626.856665: Server has referral realm; starting with
HTTP/localh...@example.com
[28575] 1531502626.85: Retrieving us...@example.com ->
krbtgt/example@example.com from FILE:/tmp/krb5cc_502 with result: 0/Success
[28575] 1531502626.856667: Starting with TGT for client realm:
us...@example.com -> krbtgt/example@example.com
[28575] 1531502626.856668: Requesting tickets for
HTTP/localh...@example.com, referrals on
[28575] 1531502626.856669: Generated subkey for TGS request: aes128-cts/86C4
[28575] 1531502626.856670: etypes requested in TGS request: aes256-cts,
aes128-cts, aes256-sha2, aes128-sha2, des3-cbc-sha1, rc4-hmac, camellia128-cts,
camellia256-cts
[28575] 1531502626.856672: Encoding request body and padata into FAST
request
[28575] 1531502626.856673: Sending request (807 bytes) to EXAMPLE.COM
[28575] 1531502626.856674: Resolving hostname localhost
[28575] 1531502626.856675: Initiating TCP connection to stream ::1:60299
[28575] 1531502626.856676: Terminating TCP connection to stream ::1:60299
[28575] 1531502626.856677: Initiating TCP connection to stream
127.0.0.1:60299
[28575] 1531502626.856678: Sending TCP request to stream 127.0.0.1:60299
[28575] 1531502626.856679: Received answer (119 bytes) from stream
127.0.0.1:60299
[28575] 1531502626.856680: Terminating TCP connection to stream
127.0.0.1:60299
[28575] 1531502626.856681: Sending DNS URI query for _kerberos.EXAMPLE.COM.
[28575] 1531502626.856682: No URI records found
[28575] 1531502626.856683: Sending DNS SRV query for
_kerberos-master._udp.EXAMPLE.COM.
[28575] 1531502626.856684: Sending DNS SRV query for
_kerberos-master._tcp.EXAMPLE.COM.
[28575] 1531502626.856685: No SRV records found
[28575] 1531502626.856686: Response was not from master KDC
[28575] 1531502626.856687: TGS request result: -1765328343/Message stream
modified
[28575] 1531502626.856688: Requesting tickets for
HTTP/localh...@example.com, referrals off
[28575] 1531502626.856689: Generated subkey for TGS request: aes128-cts/F96F
[28575] 1531502626.856690: etypes requested in TGS request: aes256-cts,
aes128-cts, aes256-sha2, aes128-sha2, des3-cbc-sha1, rc4-hmac, camellia128-cts,
camellia256-cts
[28575] 1531502626.856692: Encoding request body and padata into FAST
request
[28575] 1531502626.856693: Sending request (807 bytes) to EXAMPLE.COM
[28575] 1531502626.856694: Resolving hostname localhost
[28575] 1531502626.856695: Initiating TCP connection to stream ::1:60299
[28575] 1531502626.856696: Terminating TCP connection to stream ::1:60299
[28575] 1531502626.856697: Initiating TCP connection to stream
127.0.0.1:60299
[28575] 1531502626.856698: Sending TCP request to stream 127.0.0.1:60299
[28575] 1531502626.856699: Received answer (119 bytes) from stream
127.0.0.1:60299
[28575] 1531502626.856700: Terminating TCP connection to stream
127.0.0.1:60299
[28575] 1
[GitHub] phoenix issue #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on the issue:
https://github.com/apache/phoenix/pull/307
Ok, where I'm at now:
* Python 2.7.15 (installed via pyenv)
* Using virtualenv to circumvent the .sh script
* Modified the junit test to just leave it running
* Modified the junit test to just use the minikdc's kdc.conf
* Pulled back the pykerberos dependency to 1.1.14 to get past an "illegal
instruction error" that I get with pykerberos-1.2.1 (or whatever pip found)
This gets the phoenixdb client to actually submit the initial POST and get
the `WWW-Authenticate: Negotiate` header back. However, my client seems to be
unable to generate its challenge data from our mini kdc:
```
DEBUG:phoenixdb.avatica.client:POST http://localhost:60358/
'\n@org.apache.calcite.avatica.proto.Requests$CloseConnectionRequest\x12&\n$f71fb5c5-a814-4766-9691-8aeddfc0eea4'
{'content-type': 'application/x-google-protobuf'}
DEBUG:urllib3.connectionpool:Starting new HTTP connection (1):
localhost:60358
send: 'POST / HTTP/1.1\r\nHost: localhost:60358\r\nConnection:
keep-alive\r\nAccept-Encoding: gzip, deflate\r\nAccept: */*\r\nUser-Agent:
python-requests/2.19.1\r\ncontent-type:
application/x-google-protobuf\r\nContent-Length:
106\r\n\r\n\n@org.apache.calcite.avatica.proto.Requests$CloseConnectionRequest\x12&\n$f71fb5c5-a814-4766-9691-8aeddfc0eea4'
reply: 'HTTP/1.1 401 Unauthorized\r\n'
header: Date: Fri, 13 Jul 2018 17:06:02 GMT
header: WWW-Authenticate: Negotiate
header: Cache-Control: must-revalidate,no-cache,no-store
header: Content-Type: text/html; charset=ISO-8859-1
header: Content-Length: 281
header: Server: Jetty(9.2.19.v20160908)
DEBUG:urllib3.connectionpool:http://localhost:60358 "POST / HTTP/1.1" 401
281
DEBUG:requests_kerberos.kerberos_:handle_401(): Handling: 401
ERROR:requests_kerberos.kerberos_:generate_request_header():
authGSSClientStep() failed:
Traceback (most recent call last):
File
"/Users/jelser/projects/phoenix.git/python/requests-kerberos/requests_kerberos/kerberos_.py",
line 235, in generate_request_header
negotiate_resp_value)
GSSError: (('Unspecified GSS failure. Minor code may provide more
information', 851968), ('Message stream modified', 11))
ERROR:requests_kerberos.kerberos_:(('Unspecified GSS failure. Minor code
may provide more information', 851968), ('Message stream modified', 11))
Traceback (most recent call last):
File
"/Users/jelser/projects/phoenix.git/python/requests-kerberos/requests_kerberos/kerberos_.py",
line 235, in generate_request_header
negotiate_resp_value)
GSSError: (('Unspecified GSS failure. Minor code may provide more
information', 851968), ('Message stream modified', 11))
```
I can't seem to unwrap what's wrong with the request to the KDC which is
preventing that from happening. Need to find more debug...
---
[GitHub] phoenix issue #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/307 Ah, I think my issue might have been cruft sitting in `python/requests-kerberos/`, the `build` dir and the `.egg-info` dir. Getting a straightforward HTTP/401 error now. That I know how to deal with :) ---
[GitHub] phoenix issue #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/307 > Not sure how you got MIT on Mac OS X, is it in some ports package Yeah, homebrew has a `krb5` package which I use for running stuff locally (e.g. I put it on the `PATH` before the Heimdal, osx-provided variant) > I suppose integrating docker into the mix would make things interesting. I'll try it over the weekend if I get the time. I'm playing with different versions of python now, but am just worried about the feasibility of this actually working on the general person's machine given how much I'm struggling :\ ---
[GitHub] phoenix issue #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/307 Ugh, I'm getting frustrated: I have MIT kerberos on my Mac, so I unblocked myself first by just forcing the minikdc config file to be made instead of the if-branch you added, Lev. The next thing, I get a failure trying to launch python from the virtualenv: ``` /Users/jelser/projects/phoenix.git/phoenix-queryserver/./src/it/bin/test_phoenixdb.sh: line 59: 66933 Illegal instruction: 4 python $PYTHON_SCRIPT $PQS_PORT ``` This was reproducible doing it by hand, so I thought maybe it was related to me using python-2.7.14 (old). So, I switched over to Python-3.6.4, reinstalled everything, and I got this. ``` Traceback (most recent call last): File "/private/var/folders/4q/q02ykc2j5l1fg8nbs_sczskhgp/T/tmp.iUMwkyIZ/lib/python3.6/site-packages/requests_kerberos/kerberos_.py", line 2, in import kerberos ImportError: dlopen(/private/var/folders/4q/q02ykc2j5l1fg8nbs_sczskhgp/T/tmp.iUMwkyIZ/lib/python3.6/site-packages/kerberos.cpython-36m-darwin.so, 2): Symbol not found: _mempcpy Referenced from: /private/var/folders/4q/q02ykc2j5l1fg8nbs_sczskhgp/T/tmp.iUMwkyIZ/lib/python3.6/site-packages/kerberos.cpython-36m-darwin.so Expected in: flat namespace in /private/var/folders/4q/q02ykc2j5l1fg8nbs_sczskhgp/T/tmp.iUMwkyIZ/lib/python3.6/site-packages/kerberos.cpython-36m-darwin.so ``` I ran into another GH issue saying that pykerberos==1.1.14 fixed it for them, but I'm not seeing a difference locally. How do you feel about requiring Docker, @pu239ppy? ;) ---
[GitHub] phoenix issue #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/307 > If you are still unable to get .deactivate to work, remove it from the shell script for now and we can revisit it. Again I am being overly pedantic here, but a the shell exits, we really do not need to clean up any environment. Ok. I'm looking at this again today. Thanks for the feedback so far. The test/code looks good to me. I think we just need to update documentation to explain what we're doing (maybe a README.md in `python/`?) ---
[GitHub] phoenix pull request #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/307#discussion_r202106445
--- Diff: phoenix-queryserver/src/it/bin/test_phoenixdb.sh ---
@@ -0,0 +1,59 @@
+#/usr/bin/env bash
+
+set -u
+set -x
+set -e
+
+function cleanup {
+set +e
+set +u
+kdestroy
--- End diff --
Ok, cool. I didn't think kdestroy was doing more than just cleaning up
those token :)
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r201468535
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/ClientHashAggregatingResultIterator.java
---
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.iterate;
+
+import static org.apache.phoenix.query.QueryConstants.AGG_TIMESTAMP;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN_FAMILY;
+
+import java.io.IOException;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Map;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.aggregator.Aggregator;
+import org.apache.phoenix.expression.aggregator.Aggregators;
+import org.apache.phoenix.schema.tuple.MultiKeyValueTuple;
+import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.util.KeyValueUtil;
+import org.apache.phoenix.util.TupleUtil;
+
+/**
+ *
+ * This class implements client-side hash aggregation in memory.
+ * Issue https://issues.apache.org/jira/browse/PHOENIX-4751.
+ *
+ */
+public class ClientHashAggregatingResultIterator
+implements AggregatingResultIterator {
+
+private static final int HASH_AGG_INIT_SIZE = 64*1024;
+private static final byte[] UNITIALIZED_KEY_BUFFER = new byte[0];
+private final ResultIterator resultIterator;
+private final Aggregators aggregators;
+private final List groupByExpressions;
+private HashMap hash;
+private List keyList;
+private Iterator keyIterator;
+
+public ClientHashAggregatingResultIterator(ResultIterator
resultIterator, Aggregators aggregators, List groupByExpressions) {
+if (resultIterator == null) throw new NullPointerException();
+if (aggregators == null) throw new NullPointerException();
+if (groupByExpressions == null) throw new NullPointerException();
+this.resultIterator = resultIterator;
+this.aggregators = aggregators;
+this.groupByExpressions = groupByExpressions;
+}
+
+@Override
+public Tuple next() throws SQLException {
+if (keyIterator == null) {
+populateHash();
--- End diff --
A comment here would be nice to note the side-effect that `hash` and
`keyList` are guaranteed to be non-null (and thus, the lack of the defensive
null-checks below is OK).
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r201465583
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/ClientHashAggregatingResultIterator.java
---
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.iterate;
+
+import static org.apache.phoenix.query.QueryConstants.AGG_TIMESTAMP;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN_FAMILY;
+
+import java.io.IOException;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Map;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.aggregator.Aggregator;
+import org.apache.phoenix.expression.aggregator.Aggregators;
+import org.apache.phoenix.schema.tuple.MultiKeyValueTuple;
+import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.util.KeyValueUtil;
+import org.apache.phoenix.util.TupleUtil;
+
+/**
+ *
+ * This class implements client-side hash aggregation in memory.
+ * Issue https://issues.apache.org/jira/browse/PHOENIX-4751.
+ *
+ */
+public class ClientHashAggregatingResultIterator
+implements AggregatingResultIterator {
+
+private static final int HASH_AGG_INIT_SIZE = 64*1024;
+private static final byte[] UNITIALIZED_KEY_BUFFER = new byte[0];
+private final ResultIterator resultIterator;
+private final Aggregators aggregators;
+private final List groupByExpressions;
+private HashMap hash;
+private List keyList;
+private Iterator keyIterator;
+
+public ClientHashAggregatingResultIterator(ResultIterator
resultIterator, Aggregators aggregators, List groupByExpressions) {
+if (resultIterator == null) throw new NullPointerException();
--- End diff --
Could consolidate this down into `Objects.requireNonNull(resultIterator)`
from java.util.Objects.
---
[GitHub] phoenix issue #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/307 Tried to run the test, but it failed with: ``` 2018-07-10 13:43:55,599 ERROR [main] end2end.SecureQueryServerPhoenixDBIT(359): + . deactivate '' 2018-07-10 13:43:55,599 ERROR [main] end2end.SecureQueryServerPhoenixDBIT(359): /Users/jelser/projects/phoenix.git/phoenix-queryserver/./src/it/bin/test_phoenixdb.sh: line 12: deactivate: No such file or directory ``` Might have been me hacking on things... digging more. ---
[GitHub] phoenix issue #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/307 > How do we make sure requests-kerberos is built in a way that phoenixdb will grab it? (e.g. 0.13.0.dev0-phoenixdb) Ah, I see this is updated in python/requests-kerberos/requests_kerberos/__init__.py. We should update python/phoenixdb/requirements.txt to make sure we pull that 0.13.0.dev0-phoenixdb, right? ---
[GitHub] phoenix pull request #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/307#discussion_r201418146 --- Diff: phoenix-queryserver/pom.xml --- @@ -47,6 +47,11 @@ org.apache.maven.plugins maven-failsafe-plugin + + +**/SecureQueryServerPhoenixDBIT.java --- End diff -- You not intending for this test to be executed during the normal build process? ---
[GitHub] phoenix pull request #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/307#discussion_r201078376 --- Diff: python/requests-kerberos/LICENSE --- @@ -0,0 +1,15 @@ +ISC License --- End diff -- Just calling out that this is allowed: ISC is a Category-A license per https://www.apache.org/legal/resolved.html ---
[GitHub] phoenix pull request #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/307#discussion_r201077800 --- Diff: python/phoenixdb-module/phoenixdb/__init__.py --- @@ -1,11 +1,10 @@ -# Licensed to the Apache Software Foundation (ASF) under one or more -# contributor license agreements. See the NOTICE file distributed with -# this work for additional information regarding copyright ownership. -# The ASF licenses this file to You under the Apache License, Version 2.0 -# (the "License"); you may not use this file except in compliance with -# the License. You may obtain a copy of the License at +# Copyright 2015 Lukas Lalinsky --- End diff -- Any reason for the re-add of this? We don't need this after the IP Clearance process, I think. NOTICE file should be sufficient. ---
[GitHub] phoenix pull request #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/307#discussion_r201076404
--- Diff: phoenix-queryserver/src/it/bin/test_phoenixdb.sh ---
@@ -0,0 +1,59 @@
+#/usr/bin/env bash
+
+set -u
+set -x
+set -e
+
+function cleanup {
+set +e
+set +u
+kdestroy
--- End diff --
If we use a custom directory for the `kinit`, then this just becomes
removing that custom directory.
---
[GitHub] phoenix pull request #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/307#discussion_r201075953
--- Diff: phoenix-queryserver/src/it/bin/test_phoenixdb.sh ---
@@ -0,0 +1,59 @@
+#/usr/bin/env bash
+
+set -u
+set -x
+set -e
+
+function cleanup {
+set +e
+set +u
+kdestroy
+pushd ${PY_ENV_PATH}/bin
+. deactivate ""
+popd
+rm -rf $PY_ENV_PATH
+}
+
+trap cleanup EXIT
+
+echo "LAUNCHING SCRIPT"
+
+LOCAL_PY=$1
+PRINC=$2
+KEYTAB_LOC=$3
+KRB5_CFG_FILE=$4
+PQS_PORT=$5
+PYTHON_SCRIPT=$6
+
+PY_ENV_PATH=$( mktemp -d )
+
+conda create -y -p $PY_ENV_PATH || virtualenv $PY_ENV_PATH
+
+pushd ${PY_ENV_PATH}/bin
+
+# conda activate does stuff with unbound variables :(
+set +u
+. activate ""
+
+popd
+
+set -u
+echo "INSTALLING COMPONENTS"
+pip install -e file:///${LOCAL_PY}/requests-kerberos
+pip install -e file:///${LOCAL_PY}/phoenixdb-module
+
+export KRB5_CONFIG=$KRB5_CFG_FILE
+cat $KRB5_CONFIG
+export KRB5_TRACE=/dev/stdout
+
+#echo "RUNNING KINIT"
+kinit -kt $KEYTAB_LOC $PRINC
--- End diff --
Can we kinit to a custom location? e.g. the `-c` option. Then, later, we
just set the variable `KRB5CCNAME` in the shell ENV.
This would help prevent us from bashing the user's ticket (if they already
have one).
---
[GitHub] phoenix pull request #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/307#discussion_r201077202
--- Diff:
phoenix-queryserver/src/it/java/org/apache/phoenix/end2end/SecureQueryServerPhoenixDBIT.java
---
@@ -0,0 +1,423 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.end2end;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Maps;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.LocalHBaseCluster;
+import org.apache.hadoop.hbase.coprocessor.CoprocessorHost;
+import org.apache.hadoop.hbase.http.ssl.KeyStoreTestUtil;
+import org.apache.hadoop.hbase.security.HBaseKerberosUtils;
+import org.apache.hadoop.hbase.security.token.TokenProvider;
+import org.apache.hadoop.hbase.util.FSUtils;
+import org.apache.hadoop.hdfs.DFSConfigKeys;
+import org.apache.hadoop.http.HttpConfig;
+import org.apache.hadoop.minikdc.MiniKdc;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.security.authentication.util.KerberosName;
+import org.apache.phoenix.query.ConfigurationFactory;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.queryserver.client.ThinClientUtil;
+import org.apache.phoenix.queryserver.server.QueryServer;
+import org.apache.phoenix.util.InstanceResolver;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.*;
+import java.lang.reflect.Field;
+import java.security.PrivilegedAction;
+import java.security.PrivilegedExceptionAction;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map.Entry;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import java.nio.file.Paths;
+import java.util.Map;
+
+import static org.junit.Assert.*;
+
+@Category(NeedsOwnMiniClusterTest.class)
+public class SecureQueryServerPhoenixDBIT {
+private static final Log LOG =
LogFactory.getLog(SecureQueryServerPhoenixDBIT.class);
+
+private static final File TEMP_DIR = new File(getTempDirForClass());
+private static final File KEYTAB_DIR = new File(TEMP_DIR, "keytabs");
+private static final List USER_KEYTAB_FILES = new ArrayList<>();
+
+private static final String SPNEGO_PRINCIPAL = "HTTP/localhost";
+private static final String PQS_PRINCIPAL = "phoenixqs/localhost";
+private static final String SERVICE_PRINCIPAL =
"securecluster/localhost";
+private static File KEYTAB;
+
+private static MiniKdc KDC;
+private static HBaseTestingUtility UTIL = new HBaseTestingUtility();
+private static LocalHBaseCluster HBASE_CLUSTER;
+private static int NUM_CREATED_USERS;
+
+private static ExecutorService PQS_EXECUTOR;
+private static QueryServer PQS;
+private static int PQS_PORT;
+private static String PQS_URL;
+
+private static String getTempDirForClass() {
+StringBuilder sb = new StringBuilder(32);
+sb.append(System.getProperty("user.dir")).append(File.separator);
+sb.append("target").append(File.separator);
+sb.append(SecureQueryServerPhoenixDBIT.class.getSimpleName());
+return sb.toString();
+}
+
+private static void updateDefaultRealm() throws Exception {
+// (at
[GitHub] phoenix pull request #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/307#discussion_r201077355
--- Diff:
phoenix-queryserver/src/it/java/org/apache/phoenix/end2end/SecureQueryServerPhoenixDBIT.java
---
@@ -0,0 +1,423 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.end2end;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Maps;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.LocalHBaseCluster;
+import org.apache.hadoop.hbase.coprocessor.CoprocessorHost;
+import org.apache.hadoop.hbase.http.ssl.KeyStoreTestUtil;
+import org.apache.hadoop.hbase.security.HBaseKerberosUtils;
+import org.apache.hadoop.hbase.security.token.TokenProvider;
+import org.apache.hadoop.hbase.util.FSUtils;
+import org.apache.hadoop.hdfs.DFSConfigKeys;
+import org.apache.hadoop.http.HttpConfig;
+import org.apache.hadoop.minikdc.MiniKdc;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.security.authentication.util.KerberosName;
+import org.apache.phoenix.query.ConfigurationFactory;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.queryserver.client.ThinClientUtil;
+import org.apache.phoenix.queryserver.server.QueryServer;
+import org.apache.phoenix.util.InstanceResolver;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.*;
+import java.lang.reflect.Field;
+import java.security.PrivilegedAction;
+import java.security.PrivilegedExceptionAction;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map.Entry;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import java.nio.file.Paths;
+import java.util.Map;
+
+import static org.junit.Assert.*;
+
+@Category(NeedsOwnMiniClusterTest.class)
+public class SecureQueryServerPhoenixDBIT {
+private static final Log LOG =
LogFactory.getLog(SecureQueryServerPhoenixDBIT.class);
+
+private static final File TEMP_DIR = new File(getTempDirForClass());
+private static final File KEYTAB_DIR = new File(TEMP_DIR, "keytabs");
+private static final List USER_KEYTAB_FILES = new ArrayList<>();
+
+private static final String SPNEGO_PRINCIPAL = "HTTP/localhost";
+private static final String PQS_PRINCIPAL = "phoenixqs/localhost";
+private static final String SERVICE_PRINCIPAL =
"securecluster/localhost";
+private static File KEYTAB;
+
+private static MiniKdc KDC;
+private static HBaseTestingUtility UTIL = new HBaseTestingUtility();
+private static LocalHBaseCluster HBASE_CLUSTER;
+private static int NUM_CREATED_USERS;
+
+private static ExecutorService PQS_EXECUTOR;
+private static QueryServer PQS;
+private static int PQS_PORT;
+private static String PQS_URL;
+
+private static String getTempDirForClass() {
+StringBuilder sb = new StringBuilder(32);
+sb.append(System.getProperty("user.dir")).append(File.separator);
+sb.append("target").append(File.separator);
+sb.append(SecureQueryServerPhoenixDBIT.class.getSimpleName());
+return sb.toString();
+}
+
+private static void updateDefaultRealm() throws Exception {
+// (at
[GitHub] phoenix pull request #307: PHOENIX-4688 Kerberize python phoenixdb
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/307#discussion_r201076875
--- Diff:
phoenix-queryserver/src/it/java/org/apache/phoenix/end2end/SecureQueryServerPhoenixDBIT.java
---
@@ -0,0 +1,423 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.end2end;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Maps;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.LocalHBaseCluster;
+import org.apache.hadoop.hbase.coprocessor.CoprocessorHost;
+import org.apache.hadoop.hbase.http.ssl.KeyStoreTestUtil;
+import org.apache.hadoop.hbase.security.HBaseKerberosUtils;
+import org.apache.hadoop.hbase.security.token.TokenProvider;
+import org.apache.hadoop.hbase.util.FSUtils;
+import org.apache.hadoop.hdfs.DFSConfigKeys;
+import org.apache.hadoop.http.HttpConfig;
+import org.apache.hadoop.minikdc.MiniKdc;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.security.authentication.util.KerberosName;
+import org.apache.phoenix.query.ConfigurationFactory;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.queryserver.client.ThinClientUtil;
+import org.apache.phoenix.queryserver.server.QueryServer;
+import org.apache.phoenix.util.InstanceResolver;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.*;
+import java.lang.reflect.Field;
+import java.security.PrivilegedAction;
+import java.security.PrivilegedExceptionAction;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map.Entry;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import java.nio.file.Paths;
+import java.util.Map;
+
+import static org.junit.Assert.*;
+
+@Category(NeedsOwnMiniClusterTest.class)
+public class SecureQueryServerPhoenixDBIT {
+private static final Log LOG =
LogFactory.getLog(SecureQueryServerPhoenixDBIT.class);
+
+private static final File TEMP_DIR = new File(getTempDirForClass());
+private static final File KEYTAB_DIR = new File(TEMP_DIR, "keytabs");
+private static final List USER_KEYTAB_FILES = new ArrayList<>();
+
+private static final String SPNEGO_PRINCIPAL = "HTTP/localhost";
+private static final String PQS_PRINCIPAL = "phoenixqs/localhost";
+private static final String SERVICE_PRINCIPAL =
"securecluster/localhost";
+private static File KEYTAB;
+
+private static MiniKdc KDC;
+private static HBaseTestingUtility UTIL = new HBaseTestingUtility();
+private static LocalHBaseCluster HBASE_CLUSTER;
+private static int NUM_CREATED_USERS;
+
+private static ExecutorService PQS_EXECUTOR;
+private static QueryServer PQS;
+private static int PQS_PORT;
+private static String PQS_URL;
+
+private static String getTempDirForClass() {
+StringBuilder sb = new StringBuilder(32);
+sb.append(System.getProperty("user.dir")).append(File.separator);
+sb.append("target").append(File.separator);
+sb.append(SecureQueryServerPhoenixDBIT.class.getSimpleName());
+return sb.toString();
+}
+
+private static void updateDefaultRealm() throws Exception {
+// (at
[GitHub] phoenix pull request #277: PHOENIX-3757 System mutex table not being created...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/277#discussion_r146973563
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -3101,49 +3124,68 @@ void ensureSystemTablesUpgraded(ReadOnlyProps props)
// Regardless of the case 1 or 2, if the NS does not
exist, we will error expectedly
// below. If the NS does exist and is mapped, the below
check will exit gracefully.
}
-
+
List tableNames = getSystemTableNames(admin);
// No tables exist matching "SYSTEM\..*", they are all already
in "SYSTEM:.*"
if (tableNames.size() == 0) { return; }
// Try to move any remaining tables matching "SYSTEM\..*" into
"SYSTEM:"
if (tableNames.size() > 5) {
logger.warn("Expected 5 system tables but found " +
tableNames.size() + ":" + tableNames);
}
+
+// Try acquiring a lock in SYSMUTEX table before migrating the
tables since it involves disabling the table
+// If we cannot acquire lock, it means some old client is
either migrating SYSCAT or trying to upgrade the
+// schema of SYSCAT table and hence it should not be
interrupted
+acquiredMutexLock = acquireUpgradeMutex(0, mutexRowKey);
+if(acquiredMutexLock) {
+logger.debug("Acquired lock in SYSMUTEX table for
migrating SYSTEM tables to SYSTEM namespace");
+}
+// We will not reach here if we fail to acquire the lock,
since it throws UpgradeInProgressException
+
+// Handle the upgrade of SYSMUTEX table separately since it
doesn't have any entries in SYSCAT
+String sysMutexSrcTableName =
PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME;
+String sysMutexDestTableName =
SchemaUtil.getPhysicalName(sysMutexSrcTableName.getBytes(),
props).getNameAsString();
+UpgradeUtil.mapTableToNamespace(admin, sysMutexSrcTableName,
sysMutexDestTableName, PTableType.SYSTEM);
+
byte[] mappedSystemTable = SchemaUtil
.getPhysicalName(PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME_BYTES,
props).getName();
metatable = getTable(mappedSystemTable);
if
(tableNames.contains(PhoenixDatabaseMetaData.SYSTEM_CATALOG_HBASE_TABLE_NAME)) {
if (!admin.tableExists(mappedSystemTable)) {
+// Actual migration of SYSCAT table
UpgradeUtil.mapTableToNamespace(admin, metatable,
PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME,
props, null, PTableType.SYSTEM,
null);
+// Invalidate the client-side metadataCache
ConnectionQueryServicesImpl.this.removeTable(null,
PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME,
null,
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_1_0);
}
tableNames.remove(PhoenixDatabaseMetaData.SYSTEM_CATALOG_HBASE_TABLE_NAME);
}
-
tableNames.remove(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME);
for (TableName table : tableNames) {
UpgradeUtil.mapTableToNamespace(admin, metatable,
table.getNameAsString(), props, null, PTableType.SYSTEM,
null);
ConnectionQueryServicesImpl.this.removeTable(null,
table.getNameAsString(), null,
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_1_0);
}
-if (!tableNames.isEmpty()) {
-clearCache();
-}
+
+// Clear the server-side metadataCache when all tables are
migrated so that the new PTable can be loaded with NS mapping
+clearCache();
} finally {
if (metatable != null) {
metatable.close();
}
+if(acquiredMutexLock) {
--- End diff --
Yup, fine by me. It seems to me to be the same as it was previously :) The
mutex table update would provide exclusion for both same and different JVM
cases.
---
[GitHub] phoenix pull request #277: PHOENIX-3757 System mutex table not being created...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/277#discussion_r146928850
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -3101,49 +3124,68 @@ void ensureSystemTablesUpgraded(ReadOnlyProps props)
// Regardless of the case 1 or 2, if the NS does not
exist, we will error expectedly
// below. If the NS does exist and is mapped, the below
check will exit gracefully.
}
-
+
List tableNames = getSystemTableNames(admin);
// No tables exist matching "SYSTEM\..*", they are all already
in "SYSTEM:.*"
if (tableNames.size() == 0) { return; }
// Try to move any remaining tables matching "SYSTEM\..*" into
"SYSTEM:"
if (tableNames.size() > 5) {
logger.warn("Expected 5 system tables but found " +
tableNames.size() + ":" + tableNames);
}
+
+// Try acquiring a lock in SYSMUTEX table before migrating the
tables since it involves disabling the table
+// If we cannot acquire lock, it means some old client is
either migrating SYSCAT or trying to upgrade the
+// schema of SYSCAT table and hence it should not be
interrupted
+acquiredMutexLock = acquireUpgradeMutex(0, mutexRowKey);
+if(acquiredMutexLock) {
+logger.debug("Acquired lock in SYSMUTEX table for
migrating SYSTEM tables to SYSTEM namespace");
+}
+// We will not reach here if we fail to acquire the lock,
since it throws UpgradeInProgressException
+
+// Handle the upgrade of SYSMUTEX table separately since it
doesn't have any entries in SYSCAT
+String sysMutexSrcTableName =
PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME;
+String sysMutexDestTableName =
SchemaUtil.getPhysicalName(sysMutexSrcTableName.getBytes(),
props).getNameAsString();
+UpgradeUtil.mapTableToNamespace(admin, sysMutexSrcTableName,
sysMutexDestTableName, PTableType.SYSTEM);
+
byte[] mappedSystemTable = SchemaUtil
.getPhysicalName(PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME_BYTES,
props).getName();
metatable = getTable(mappedSystemTable);
if
(tableNames.contains(PhoenixDatabaseMetaData.SYSTEM_CATALOG_HBASE_TABLE_NAME)) {
if (!admin.tableExists(mappedSystemTable)) {
+// Actual migration of SYSCAT table
UpgradeUtil.mapTableToNamespace(admin, metatable,
PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME,
props, null, PTableType.SYSTEM,
null);
+// Invalidate the client-side metadataCache
ConnectionQueryServicesImpl.this.removeTable(null,
PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME,
null,
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_1_0);
}
tableNames.remove(PhoenixDatabaseMetaData.SYSTEM_CATALOG_HBASE_TABLE_NAME);
}
-
tableNames.remove(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME);
for (TableName table : tableNames) {
UpgradeUtil.mapTableToNamespace(admin, metatable,
table.getNameAsString(), props, null, PTableType.SYSTEM,
null);
ConnectionQueryServicesImpl.this.removeTable(null,
table.getNameAsString(), null,
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_1_0);
}
-if (!tableNames.isEmpty()) {
-clearCache();
-}
+
+// Clear the server-side metadataCache when all tables are
migrated so that the new PTable can be loaded with NS mapping
+clearCache();
} finally {
if (metatable != null) {
metatable.close();
}
+if(acquiredMutexLock) {
--- End diff --
If there isn't any mutual exclusion happening at a higher level, most
definitely we should make sure this isn't happening concurrently.
What about clients in separate JVMs though? What's the "prior art" for the
rest of the SYSTEM tables?
---
[GitHub] phoenix pull request #277: PHOENIX-3757 System mutex table not being created...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/277#discussion_r146902970
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -3101,49 +3124,72 @@ void ensureSystemTablesUpgraded(ReadOnlyProps props)
// Regardless of the case 1 or 2, if the NS does not
exist, we will error expectedly
// below. If the NS does exist and is mapped, the below
check will exit gracefully.
}
-
+
List tableNames = getSystemTableNames(admin);
// No tables exist matching "SYSTEM\..*", they are all already
in "SYSTEM:.*"
if (tableNames.size() == 0) { return; }
// Try to move any remaining tables matching "SYSTEM\..*" into
"SYSTEM:"
if (tableNames.size() > 5) {
logger.warn("Expected 5 system tables but found " +
tableNames.size() + ":" + tableNames);
}
+
+// Try acquiring a lock in SYSMUTEX table before migrating the
tables since it involves disabling the table
+// If we cannot acquire lock, it means some old client is
either migrating SYSCAT or trying to upgrade the
+// schema of SYSCAT table and hence it should not be
interrupted
+acquiredMutexLock = acquireUpgradeMutex(0, mutexRowKey);
+if(acquiredMutexLock) {
+logger.debug("Acquired lock in SYSMUTEX table for
migrating SYSTEM tables to SYSTEM namespace");
--- End diff --
Log a debug message for the `else` case too? Easier than finding the lack
of the above message when debugging..
---
[GitHub] phoenix pull request #277: PHOENIX-3757 System mutex table not being created...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/277#discussion_r146903705
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -3101,49 +3124,68 @@ void ensureSystemTablesUpgraded(ReadOnlyProps props)
// Regardless of the case 1 or 2, if the NS does not
exist, we will error expectedly
// below. If the NS does exist and is mapped, the below
check will exit gracefully.
}
-
+
List tableNames = getSystemTableNames(admin);
// No tables exist matching "SYSTEM\..*", they are all already
in "SYSTEM:.*"
if (tableNames.size() == 0) { return; }
// Try to move any remaining tables matching "SYSTEM\..*" into
"SYSTEM:"
if (tableNames.size() > 5) {
logger.warn("Expected 5 system tables but found " +
tableNames.size() + ":" + tableNames);
}
+
+// Try acquiring a lock in SYSMUTEX table before migrating the
tables since it involves disabling the table
+// If we cannot acquire lock, it means some old client is
either migrating SYSCAT or trying to upgrade the
+// schema of SYSCAT table and hence it should not be
interrupted
+acquiredMutexLock = acquireUpgradeMutex(0, mutexRowKey);
+if(acquiredMutexLock) {
+logger.debug("Acquired lock in SYSMUTEX table for
migrating SYSTEM tables to SYSTEM namespace");
+}
+// We will not reach here if we fail to acquire the lock,
since it throws UpgradeInProgressException
+
+// Handle the upgrade of SYSMUTEX table separately since it
doesn't have any entries in SYSCAT
+String sysMutexSrcTableName =
PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME;
+String sysMutexDestTableName =
SchemaUtil.getPhysicalName(sysMutexSrcTableName.getBytes(),
props).getNameAsString();
+UpgradeUtil.mapTableToNamespace(admin, sysMutexSrcTableName,
sysMutexDestTableName, PTableType.SYSTEM);
+
byte[] mappedSystemTable = SchemaUtil
.getPhysicalName(PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME_BYTES,
props).getName();
metatable = getTable(mappedSystemTable);
if
(tableNames.contains(PhoenixDatabaseMetaData.SYSTEM_CATALOG_HBASE_TABLE_NAME)) {
if (!admin.tableExists(mappedSystemTable)) {
+// Actual migration of SYSCAT table
UpgradeUtil.mapTableToNamespace(admin, metatable,
PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME,
props, null, PTableType.SYSTEM,
null);
+// Invalidate the client-side metadataCache
ConnectionQueryServicesImpl.this.removeTable(null,
PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME,
null,
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_1_0);
}
tableNames.remove(PhoenixDatabaseMetaData.SYSTEM_CATALOG_HBASE_TABLE_NAME);
}
-
tableNames.remove(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME);
for (TableName table : tableNames) {
UpgradeUtil.mapTableToNamespace(admin, metatable,
table.getNameAsString(), props, null, PTableType.SYSTEM,
null);
ConnectionQueryServicesImpl.this.removeTable(null,
table.getNameAsString(), null,
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_1_0);
}
-if (!tableNames.isEmpty()) {
-clearCache();
-}
+
+// Clear the server-side metadataCache when all tables are
migrated so that the new PTable can be loaded with NS mapping
+clearCache();
--- End diff --
> Also, we clear server side cache anytime a SYSTEM table is disabled,
using a coprocessor hook.
My assumption was that the intent was to drop any cached PTables if we
moved any system tables around. Given the above, it sounds like this is (now?)
unnecessary.
> So I decided to simplify the logic and clear the cache every time. Its
good to do that because the server side cache will go out of sync with the
SYSCAT table once the migration happens.
Is that being down in a CP too? If so, would it make sense to do this
cache-clearing there (instead of client side)?
---
[GitHub] phoenix pull request #277: PHOENIX-3757 System mutex table not being created...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/277#discussion_r146902673
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -3086,12 +3103,18 @@ private void restoreFromSnapshot(String tableName,
String snapshotName,
}
}
-void ensureSystemTablesUpgraded(ReadOnlyProps props)
+void ensureSystemTablesMigratedToSystemNamespace(ReadOnlyProps props)
throws SQLException, IOException, IllegalArgumentException,
InterruptedException {
if (!SchemaUtil.isNamespaceMappingEnabled(PTableType.SYSTEM,
props)) { return; }
+
+boolean acquiredMutexLock = false;
+byte[] mutexRowKey = SchemaUtil.getTableKey(null,
PhoenixDatabaseMetaData.SYSTEM_CATALOG_SCHEMA,
+PhoenixDatabaseMetaData.SYSTEM_CATALOG_TABLE);
+
HTableInterface metatable = null;
try (HBaseAdmin admin = getAdmin()) {
-// Namespace-mapping is enabled at this point.
+ // SYSTEM namespace needs to be created via HBase API's
because "CREATE SCHEMA" statement tries to write its metadata
+ // in SYSTEM:CATALOG table. Without SYSTEM namespace,
SYSTEM:CATALOG table cannot be created.
--- End diff --
nice comments here and elsewhere in this method.
---
[GitHub] phoenix pull request #277: PHOENIX-3757 System mutex table not being created...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/277#discussion_r146902373
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -2526,8 +2541,14 @@ private void
createOtherSystemTables(PhoenixConnection metaConnection) throws SQ
try {
metaConnection.createStatement().execute(QueryConstants.CREATE_FUNCTION_METADATA);
} catch (TableAlreadyExistsException ignore) {}
+// We catch TableExistsException in createSysMutexTable() and
ignore it. Hence we will also ignore IOException here.
+// SYSTEM.MUTEX table should not be exposed to user. Hence it is
directly created and used via HBase API.
+// Using 'CREATE TABLE' statement will add entries to
SYSTEM.CATALOG table, which should not happen.
+try {
+createSysMutexTable(hBaseAdmin,
ConnectionQueryServicesImpl.this.getProps());
+} catch (IOException ignore) {}
--- End diff --
What about the case where the table creation failed for a legitimate system
reason (something an admin needs to correct)? We should at least have some
warning to the user that we failed to created the table, right?
---
[GitHub] phoenix pull request #277: PHOENIX-3757 System mutex table not being created...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/277#discussion_r146904748
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -3195,6 +3255,18 @@ public boolean releaseUpgradeMutex(byte[]
mutexRowKey) {
return released;
}
+private byte[] getSysMutexPhysicalTableNameBytes() throws IOException,
SQLException {
+byte[] sysMutexPhysicalTableNameBytes = null;
+try(HBaseAdmin admin = getAdmin()) {
+
if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES)) {
--- End diff --
I assume `PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES` is `byte[]`?
Would be better to use the `TableName` variable to avoid deprecated API warning.
---
[GitHub] phoenix pull request #277: PHOENIX-3757 System mutex table not being created...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/277#discussion_r146904346
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -3101,49 +3124,68 @@ void ensureSystemTablesUpgraded(ReadOnlyProps props)
// Regardless of the case 1 or 2, if the NS does not
exist, we will error expectedly
// below. If the NS does exist and is mapped, the below
check will exit gracefully.
}
-
+
List tableNames = getSystemTableNames(admin);
// No tables exist matching "SYSTEM\..*", they are all already
in "SYSTEM:.*"
if (tableNames.size() == 0) { return; }
// Try to move any remaining tables matching "SYSTEM\..*" into
"SYSTEM:"
if (tableNames.size() > 5) {
logger.warn("Expected 5 system tables but found " +
tableNames.size() + ":" + tableNames);
}
+
+// Try acquiring a lock in SYSMUTEX table before migrating the
tables since it involves disabling the table
+// If we cannot acquire lock, it means some old client is
either migrating SYSCAT or trying to upgrade the
+// schema of SYSCAT table and hence it should not be
interrupted
+acquiredMutexLock = acquireUpgradeMutex(0, mutexRowKey);
+if(acquiredMutexLock) {
+logger.debug("Acquired lock in SYSMUTEX table for
migrating SYSTEM tables to SYSTEM namespace");
+}
+// We will not reach here if we fail to acquire the lock,
since it throws UpgradeInProgressException
+
+// Handle the upgrade of SYSMUTEX table separately since it
doesn't have any entries in SYSCAT
+String sysMutexSrcTableName =
PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME;
+String sysMutexDestTableName =
SchemaUtil.getPhysicalName(sysMutexSrcTableName.getBytes(),
props).getNameAsString();
+UpgradeUtil.mapTableToNamespace(admin, sysMutexSrcTableName,
sysMutexDestTableName, PTableType.SYSTEM);
+
byte[] mappedSystemTable = SchemaUtil
.getPhysicalName(PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME_BYTES,
props).getName();
metatable = getTable(mappedSystemTable);
if
(tableNames.contains(PhoenixDatabaseMetaData.SYSTEM_CATALOG_HBASE_TABLE_NAME)) {
if (!admin.tableExists(mappedSystemTable)) {
+// Actual migration of SYSCAT table
UpgradeUtil.mapTableToNamespace(admin, metatable,
PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME,
props, null, PTableType.SYSTEM,
null);
+// Invalidate the client-side metadataCache
ConnectionQueryServicesImpl.this.removeTable(null,
PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME,
null,
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_1_0);
}
tableNames.remove(PhoenixDatabaseMetaData.SYSTEM_CATALOG_HBASE_TABLE_NAME);
}
-
tableNames.remove(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME);
for (TableName table : tableNames) {
UpgradeUtil.mapTableToNamespace(admin, metatable,
table.getNameAsString(), props, null, PTableType.SYSTEM,
null);
ConnectionQueryServicesImpl.this.removeTable(null,
table.getNameAsString(), null,
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_1_0);
}
-if (!tableNames.isEmpty()) {
-clearCache();
-}
+
+// Clear the server-side metadataCache when all tables are
migrated so that the new PTable can be loaded with NS mapping
+clearCache();
} finally {
if (metatable != null) {
metatable.close();
}
+if(acquiredMutexLock) {
--- End diff --
I have no good explanation for why it was the way it was when I last looked
at it. I can only assumed I left it as it was (really, just extracted part of
this one method into another). If you think it should also be marked
synchronized, go for it.
---
[GitHub] phoenix pull request #277: PHOENIX-3757 System mutex table not being created...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/277#discussion_r146901856
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -2514,7 +2529,7 @@ private void createSysMutexTable(HBaseAdmin admin)
throws IOException, SQLExcept
return
Lists.newArrayList(admin.listTableNames(QueryConstants.SYSTEM_SCHEMA_NAME +
"\\..*"));
}
-private void createOtherSystemTables(PhoenixConnection metaConnection)
throws SQLException {
+private void createOtherSystemTables(PhoenixConnection metaConnection,
HBaseAdmin hBaseAdmin) throws SQLException {
--- End diff --
nit: s/hBaseAdmin/hbaseAdmin/ camel-case typically isn't observed on
"hbase" ;)
---
[GitHub] phoenix pull request #275: PHOENIX-4237: Add function to calculate Java coll...
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/275#discussion_r144414821 --- Diff: phoenix-core/src/main/java/com/force/db/i18n/OracleUpper.java --- @@ -0,0 +1,66 @@ +/* --- End diff -- Yup! You got it right, James. Whether we include the code in binary form or source form, for BSD, we treat them the same (propagate in LICENSE, and copyright/etc in NOTICE). If there's a license header for the file, we would also leave that, IIRC. ---
[GitHub] phoenix issue #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/236 Thanks, Rahul. I'll add this to my list to try to merge in so we can keep moving forward! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix issue #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/236 Thanks for the fixes in f042ca0, @rahulsIOT. Github is telling me that you have some conflicts that need resolving. If you can do that and file the JIRA case about firming up the ACL support, I'll try to test+merge this today before the week ramps up. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/236#discussion_r130726475 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServicesOptions.java --- @@ -284,14 +288,18 @@ public static final int DEFAULT_COLUMN_ENCODED_BYTES = QualifierEncodingScheme.TWO_BYTE_QUALIFIERS.getSerializedMetadataValue(); public static final String DEFAULT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.SINGLE_CELL_ARRAY_WITH_OFFSETS.toString(); public static final String DEFAULT_MULTITENANT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.ONE_CELL_PER_COLUMN.toString(); +public final static String DEFAULT_PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH = "/phoenix"; +public final static String DEFAULT_PHOENIX_QUERY_SERVER_SERVICE_NAME = "queryserver"; +public final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_USERNAME = "phoenixuser"; +public final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_PASSWORD = "Xsjdhxsd"; --- End diff -- > I replied saying that we can close this PR and open another ticket, where I will fix the SASL ACLs. Implementing SASL acls is one thing -- my above comment was about not forgetting to document the lack of initial support (much less the feature itself). We should make sure that a documentation issue gets filed, not that you build something that rots because no one knows how to use it! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix issue #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/236 > The LoadBalancer.getLoadBalancer().getSingleServiceLocation() throw exception that client needs to catch. Usually it shows that there is some issue with connecting to zookeeper or there is not PQS registered with the zookeeper. Also thinking about this after the review comment about configuring the digest auth ACL for ZK, we'll need to get together documentation for this feature. This is another detail to add to the list of things to cover :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/236#discussion_r129067645 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServicesOptions.java --- @@ -284,14 +288,18 @@ public static final int DEFAULT_COLUMN_ENCODED_BYTES = QualifierEncodingScheme.TWO_BYTE_QUALIFIERS.getSerializedMetadataValue(); public static final String DEFAULT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.SINGLE_CELL_ARRAY_WITH_OFFSETS.toString(); public static final String DEFAULT_MULTITENANT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.ONE_CELL_PER_COLUMN.toString(); +public final static String DEFAULT_PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH = "/phoenix"; +public final static String DEFAULT_PHOENIX_QUERY_SERVER_SERVICE_NAME = "queryserver"; +public final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_USERNAME = "phoenixuser"; +public final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_PASSWORD = "Xsjdhxsd"; --- End diff -- I think we should just change this to be something simple like "phoenix/phoenix" instead of "phoenixuser/Xsjdhxsd" (the public random text is no-more secure than an easily remember-able string). We also need to document how users are expected to secure this (property to set in hbase-site.xml, make sure hbase-site.xml is not globally readable, etc). I'm seeing down below that there is not support for SASL ACLs -- that will also need to be documented as a limitation. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/236#discussion_r129066492 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java --- @@ -255,9 +255,16 @@ public static final String DEFAULT_COLUMN_ENCODED_BYTES_ATRRIB = "phoenix.default.column.encoded.bytes.attrib"; public static final String DEFAULT_IMMUTABLE_STORAGE_SCHEME_ATTRIB = "phoenix.default.immutable.storage.scheme"; public static final String DEFAULT_MULTITENANT_IMMUTABLE_STORAGE_SCHEME_ATTRIB = "phoenix.default.multitenant.immutable.storage.scheme"; + +public final static String PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH = "phoenix.queryserver.base.path"; +public final static String PHOENIX_QUERY_SERVER_SERVICE_NAME = "phoenix.queryserver.service.name"; +public final static String PHOENIX_QUERY_SERVER_ZK_ACL_USERNAME = "phoenix.queryserver.zookeeper.acl.username"; +public final static String PHOENIX_QUERY_SERVER_ZK_ACL_PASSWORD = "phoenix.queryserver.zookeeper.acl.password"; public static final String STATS_COLLECTION_ENABLED = "phoenix.stats.collection.enabled"; public static final String USE_STATS_FOR_PARALLELIZATION = "phoenix.use.stats.parallelization"; + + --- End diff -- nit: remove please. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/236#discussion_r129067705 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServicesOptions.java --- @@ -284,14 +288,18 @@ public static final int DEFAULT_COLUMN_ENCODED_BYTES = QualifierEncodingScheme.TWO_BYTE_QUALIFIERS.getSerializedMetadataValue(); public static final String DEFAULT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.SINGLE_CELL_ARRAY_WITH_OFFSETS.toString(); public static final String DEFAULT_MULTITENANT_IMMUTABLE_STORAGE_SCHEME = ImmutableStorageScheme.ONE_CELL_PER_COLUMN.toString(); +public final static String DEFAULT_PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH = "/phoenix"; +public final static String DEFAULT_PHOENIX_QUERY_SERVER_SERVICE_NAME = "queryserver"; +public final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_USERNAME = "phoenixuser"; +public final static String DEFAULT_PHOENIX_QUERY_SERVER_ZK_ACL_PASSWORD = "Xsjdhxsd"; //by default, max connections from one client to one cluster is unlimited public static final int DEFAULT_CLIENT_CONNECTION_MAX_ALLOWED_CONNECTIONS = 0; -public static final boolean DEFAULT_STATS_COLLECTION_ENABLED = true; -public static final boolean DEFAULT_USE_STATS_FOR_PARALLELIZATION = true; //default update cache frequency public static final int DEFAULT_UPDATE_CACHE_FREQUENCY = 0; +public static final boolean DEFAULT_STATS_COLLECTION_ENABLED = true; +public static final boolean DEFAULT_USE_STATS_FOR_PARALLELIZATION = true; --- End diff -- Nit: undo this move please. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/236#discussion_r129067998 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java --- @@ -255,9 +255,16 @@ public static final String DEFAULT_COLUMN_ENCODED_BYTES_ATRRIB = "phoenix.default.column.encoded.bytes.attrib"; public static final String DEFAULT_IMMUTABLE_STORAGE_SCHEME_ATTRIB = "phoenix.default.immutable.storage.scheme"; public static final String DEFAULT_MULTITENANT_IMMUTABLE_STORAGE_SCHEME_ATTRIB = "phoenix.default.multitenant.immutable.storage.scheme"; + +public final static String PHOENIX_QUERY_SERVER_CLUSTER_BASE_PATH = "phoenix.queryserver.base.path"; +public final static String PHOENIX_QUERY_SERVER_SERVICE_NAME = "phoenix.queryserver.service.name"; +public final static String PHOENIX_QUERY_SERVER_ZK_ACL_USERNAME = "phoenix.queryserver.zookeeper.acl.username"; +public final static String PHOENIX_QUERY_SERVER_ZK_ACL_PASSWORD = "phoenix.queryserver.zookeeper.acl.password"; --- End diff -- Nit: please match the style "public static final" not "public final static" (emphasis also on the two spaces) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/236#discussion_r129070107 --- Diff: phoenix-queryserver/pom.xml --- @@ -176,5 +176,10 @@ test-jar test + + org.mockito + mockito-all --- End diff -- This doesn't appear to be used to me. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/236#discussion_r129069369
--- Diff:
phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/register/Registry.java
---
@@ -0,0 +1,31 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.queryserver.register;
+
+
+import org.apache.phoenix.loadbalancer.service.LoadBalanceZookeeperConf;
+
+public interface Registry {
+
+public void deRegisterTheServer() throws Exception;
--- End diff --
nit: `deregisterServer()` instead. Or, should it be `deregisterServers()`?
Does it remove all servers since it has no arguments? (javadoc here would also
be nice :))
Also, you can drop the `public` modifier -- this is unnecessary for
interfaces.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/236#discussion_r129068996
--- Diff:
phoenix-load-balancer/src/main/java/org/apache/phoenix/queryserver/register/ZookeeperRegistry.java
---
@@ -0,0 +1,72 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.queryserver.register;
+
+
+import com.google.common.net.HostAndPort;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.curator.framework.CuratorFrameworkFactory;
+import org.apache.curator.retry.ExponentialBackoffRetry;
+import org.apache.curator.utils.CloseableUtils;
+import org.apache.phoenix.loadbalancer.service.LoadBalanceZookeeperConf;
+import org.apache.phoenix.queryserver.server.QueryServer;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.data.Stat;
+
+import java.nio.charset.StandardCharsets;
+
+
+public class ZookeeperRegistry implements Registry {
+
+protected static final Log LOG = LogFactory.getLog(QueryServer.class);
+private CuratorFramework client;
+
+public ZookeeperRegistry(){}
+
+@Override
+public void registerServer(LoadBalanceZookeeperConf configuration,
int pqsPort,
+String zookeeperConnectString, String
pqsHost)
+throws Exception {
+
+this.client =
CuratorFrameworkFactory.newClient(zookeeperConnectString,
+new ExponentialBackoffRetry(1000,10));
+this.client.start();
+HostAndPort hostAndPort = HostAndPort.fromParts(pqsHost,pqsPort);
+String path = configuration.getFullPathToNode(hostAndPort);
+String node = hostAndPort.toString();
+
this.client.create().creatingParentsIfNeeded().withMode(CreateMode.EPHEMERAL).forPath(path
+,node.getBytes(StandardCharsets.UTF_8));
+Stat stat =
this.client.setACL().withACL(configuration.getAcls()).forPath(path);
+if (stat != null) {
+LOG.info(" node created with right ACL");
+}
+else {
+LOG.error("could not create node with right ACL. So, system
would exit now.");
+System.exit(-1);
--- End diff --
Calling `System.exit(-1)` is pretty brutal for anyone trying to integrate
with your software :)
Throwing a RuntimeException would be nicer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/236#discussion_r129068605 --- Diff: phoenix-load-balancer/pom.xml --- @@ -0,0 +1,84 @@ + + + +http://maven.apache.org/POM/4.0.0; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd;> + 4.0.0 + +org.apache.phoenix +phoenix +4.11.0-HBase-1.3-SNAPSHOT + + phoenix-load-balancer + Phoenix Load Balancer + A Load balancer which routes calls to Phoenix Query Server + + + + org.apache.hbase + hbase-common --- End diff -- It looks like you're using a bunch of dependencies transitively. This will work for now, but is brittle in the long term. We should make sure that this module depends directly on all of the dependencies that it uses instead of relying on another dependency bringing them in. Can do this later though. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix issue #265: PHOENIX-3598
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/265 Woops. I forgot to close this via commit message. If you could close it at your convenience, @Wancy, I'd appreciate it! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #265: PHOENIX-3598
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/265#discussion_r124409282
--- Diff:
phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java
---
@@ -228,7 +235,9 @@ public int run(String[] args) throws Exception {
builder.withSpnego(ugi.getUserName(), additionalAllowedRealms)
.withAutomaticLogin(keytab)
.withImpersonation(new PhoenixDoAsCallback(ugi, getConf()));
+
}
+ setRemoteUserExtractorIfNecessary(builder, getConf());
--- End diff --
With respect to my long-winded comment below, if you're only looking to
support Kerberos, we want to move this line into the above if-block.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #265: PHOENIX-3598
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/265#discussion_r124409157
--- Diff:
phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java
---
@@ -273,6 +282,54 @@ public int run(String[] args) throws Exception {
}
}
+ // add remoteUserExtractor to builder if enabled
+ @VisibleForTesting
+ public void setRemoteUserExtractorIfNecessary(HttpServer.Builder
builder, Configuration conf) {
+if
(conf.getBoolean(QueryServices.QUERY_SERVER_WITH_REMOTEUSEREXTRACTOR_ATTRIB,
+
QueryServicesOptions.DEFAULT_QUERY_SERVER_WITH_REMOTEUSEREXTRACTOR)) {
+ builder.withRemoteUserExtractor(new
PhoenixRemoteUserExtractor(conf));
+}
+ }
+
+ /**
+ * Use the correctly way to extract end user.
+ */
+
+ static class PhoenixRemoteUserExtractor implements RemoteUserExtractor{
+private final HttpQueryStringParameterRemoteUserExtractor
paramRemoteUserExtractor;
+private final HttpRequestRemoteUserExtractor
requestRemoteUserExtractor;
+private final String userExtractParam;
+
+public PhoenixRemoteUserExtractor(Configuration conf) {
+ this.requestRemoteUserExtractor = new
HttpRequestRemoteUserExtractor();
+ this.userExtractParam =
conf.get(QueryServices.QUERY_SERVER_REMOTEUSEREXTRACTOR_PARAM,
+
QueryServicesOptions.DEFAULT_QUERY_SERVER_REMOTEUSEREXTRACTOR_PARAM);
+ this.paramRemoteUserExtractor = new
HttpQueryStringParameterRemoteUserExtractor(userExtractParam);
+}
+
+@Override
+public String extract(HttpServletRequest request) throws
RemoteUserExtractionException {
+ if (request.getParameter(userExtractParam) != null) {
+String extractedUser = paramRemoteUserExtractor.extract(request);
+UserGroupInformation ugi =
UserGroupInformation.createRemoteUser(request.getRemoteUser());
+UserGroupInformation proxyUser =
UserGroupInformation.createProxyUser(extractedUser, ugi);
--- End diff --
In re-reading the above, I'm a little worried about the edge-cases. With
PQS right now, we have the following cases "supported"
1) Kerberos+SPNEGO as the Kerberos user (els...@example.com authenticates
to PQS and the PQS credentials are used to query Phoenix as els...@example.com)
2) Kerberos auth with HBase but no SPNEGO for PQS clients (legacy support
for how things used to work before the SPNEGO auth was built -- PQS user does
everything for users)
3) Without Kerberos, all queries run as the PQS user (out of the box).
Avatica supports more than what point 3 above does, but we haven't
prioritized wiring that up as most people just want point 1. @Wancy, I had
originally thought you were just trying to enable a PQS client with Kerberos
credentials to say that they are someone else (extension of point 1 --
Credentials to PQS are for "elserj" but instead of querying Phoenix as
"elserj", query as "bob").
Did you also want this to work for cases when Kerberos is not in the mix? I
think that would require some additional changes as I don't think this will
work as-is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #265: PHOENIX-3598
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/265#discussion_r124402049
--- Diff:
phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java
---
@@ -273,6 +282,54 @@ public int run(String[] args) throws Exception {
}
}
+ // add remoteUserExtractor to builder if enabled
+ @VisibleForTesting
+ public void setRemoteUserExtractorIfNecessary(HttpServer.Builder
builder, Configuration conf) {
+if
(conf.getBoolean(QueryServices.QUERY_SERVER_WITH_REMOTEUSEREXTRACTOR_ATTRIB,
+
QueryServicesOptions.DEFAULT_QUERY_SERVER_WITH_REMOTEUSEREXTRACTOR)) {
+ builder.withRemoteUserExtractor(new
PhoenixRemoteUserExtractor(conf));
+}
+ }
+
+ /**
+ * Use the correctly way to extract end user.
+ */
+
+ static class PhoenixRemoteUserExtractor implements RemoteUserExtractor{
+private final HttpQueryStringParameterRemoteUserExtractor
paramRemoteUserExtractor;
+private final HttpRequestRemoteUserExtractor
requestRemoteUserExtractor;
+private final String userExtractParam;
+
+public PhoenixRemoteUserExtractor(Configuration conf) {
+ this.requestRemoteUserExtractor = new
HttpRequestRemoteUserExtractor();
+ this.userExtractParam =
conf.get(QueryServices.QUERY_SERVER_REMOTEUSEREXTRACTOR_PARAM,
+
QueryServicesOptions.DEFAULT_QUERY_SERVER_REMOTEUSEREXTRACTOR_PARAM);
+ this.paramRemoteUserExtractor = new
HttpQueryStringParameterRemoteUserExtractor(userExtractParam);
+}
+
+@Override
+public String extract(HttpServletRequest request) throws
RemoteUserExtractionException {
+ if (request.getParameter(userExtractParam) != null) {
--- End diff --
We should put a `requestRemoteUserExtractor.extract(request)` at the top of
this method implementation. We should be using it in both branches of the
conditional (replacing the `request.getRemoteUser()` call you have below)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #265: PHOENIX-3598
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/265#discussion_r124312271
--- Diff:
phoenix-queryserver/src/test/java/org/apache/phoenix/queryserver/server/PhoenixRemoteUserExtractorTest.java
---
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.queryserver.server;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import org.apache.calcite.avatica.server.RemoteUserExtractionException;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.security.authorize.AuthorizationException;
+import org.apache.hadoop.security.authorize.ProxyUsers;
+import
org.apache.phoenix.queryserver.server.QueryServer.PhoenixRemoteUserExtractor;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.http.HttpServletRequest;
+
+/**
+ * Tests for the RemoteUserExtractor Method Avatica provides for Phoenix
to implement.
+ */
+public class PhoenixRemoteUserExtractorTest {
+ private static final Logger LOG =
LoggerFactory.getLogger(PhoenixRemoteUserExtractorTest.class);
+
+ @Test
+ public void testUseDoAsSuccess() {
+HttpServletRequest request = mock(HttpServletRequest.class);
+when(request.getRemoteUser()).thenReturn("proxyserver");
+when(request.getParameter("doAs")).thenReturn("enduser");
+when(request.getRemoteAddr()).thenReturn("localhost:1234");
+
+Configuration conf = new Configuration(false);
+conf.set("hadoop.proxyuser.proxyserver.groups", "*");
+conf.set("hadoop.proxyuser.proxyserver.hosts", "*");
+conf.set("phoenix.queryserver.doAs.enabled", "true");
+ProxyUsers.refreshSuperUserGroupsConfiguration(conf);
+
+PhoenixRemoteUserExtractor extractor = new
PhoenixRemoteUserExtractor(conf);
+try {
+ assertEquals("enduser", extractor.extract(request));
+} catch (RemoteUserExtractionException e) {
+ LOG.info(e.getMessage());
+}
+ }
+
+ @Test
+ public void testDoNotUseDoAs() {
--- End diff --
No, there is no getter on the builder to verify it's called. Instead you
can use the `Mockito.verify(builder)` method. Something like:
```java
Configuration conf = createTestConfiguration();
Builder b = Mockito.mock(Builder.class);
Mockito.when(b.withRemoteUserExtractor(Mockito.any(PhoenixRemoteUserExtractor.class))).thenReturn(b);
setRemoteUserExtractorIfNecessary(b, conf);
Mockito.verify(b);
```
This should essentially verify that `withRemoteUserExtractor` was invoked
by `setRemoteUserExtractorIfNecessary`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #265: PHOENIX-3598
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/265#discussion_r124113023
--- Diff:
phoenix-queryserver/src/test/java/org/apache/phoenix/queryserver/server/PhoenixRemoteUserExtractorTest.java
---
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.queryserver.server;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import org.apache.calcite.avatica.server.RemoteUserExtractionException;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.security.authorize.AuthorizationException;
+import org.apache.hadoop.security.authorize.ProxyUsers;
+import
org.apache.phoenix.queryserver.server.QueryServer.PhoenixRemoteUserExtractor;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.http.HttpServletRequest;
+
+/**
+ * Tests for the RemoteUserExtractor Method Avatica provides for Phoenix
to implement.
+ */
+public class PhoenixRemoteUserExtractorTest {
+ private static final Logger LOG =
LoggerFactory.getLogger(PhoenixRemoteUserExtractorTest.class);
+
+ @Test
+ public void testUseDoAsSuccess() {
+HttpServletRequest request = mock(HttpServletRequest.class);
+when(request.getRemoteUser()).thenReturn("proxyserver");
+when(request.getParameter("doAs")).thenReturn("enduser");
+when(request.getRemoteAddr()).thenReturn("localhost:1234");
+
+Configuration conf = new Configuration(false);
+conf.set("hadoop.proxyuser.proxyserver.groups", "*");
+conf.set("hadoop.proxyuser.proxyserver.hosts", "*");
+conf.set("phoenix.queryserver.doAs.enabled", "true");
+ProxyUsers.refreshSuperUserGroupsConfiguration(conf);
+
+PhoenixRemoteUserExtractor extractor = new
PhoenixRemoteUserExtractor(conf);
+try {
+ assertEquals("enduser", extractor.extract(request));
+} catch (RemoteUserExtractionException e) {
+ LOG.info(e.getMessage());
+}
+ }
+
+ @Test
+ public void testDoNotUseDoAs() {
--- End diff --
To test this code if you take my above suggestion, you could make a new
method in QueryServer which does
```java
Builder setRemoteUserExtractorIfNecessary(Builder b, Configuration conf) {
if (conf.getBoolean(QueryServices.QUERY_SERVER_DOAS_ENABLED_ATTRIB,
QueryServicesOptions.DEFAULT_QUERY_SERVER_DOAS_ENABLED)) {
return builder.withRemoteUserExtractor(new
PhoenixRemoteUserExtractor(getConf()));
}
return builder;
}
```
This would let you easily mock the Builder and verify that your extractor
is configured when the property is set to "true".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #265: PHOENIX-3598
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/265#discussion_r124112286
--- Diff:
phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java
---
@@ -274,6 +282,47 @@ public int run(String[] args) throws Exception {
}
/**
+ * Use the correctly way to extract end user.
+ */
+
+ static class PhoenixRemoteUserExtractor implements RemoteUserExtractor{
+private final HttpQueryStringParameterRemoteUserExtractor
paramRemoteUserExtractor;
+private final HttpRequestRemoteUserExtractor
requestRemoteUserExtractor;
+private final boolean enableDoAs;
+private final String doAsParam;
+
+public PhoenixRemoteUserExtractor(Configuration conf) {
+ this.requestRemoteUserExtractor = new
HttpRequestRemoteUserExtractor();
+ this.doAsParam = conf.get(QueryServices.QUERY_SERVER_DOAS_PARAM,
+ QueryServicesOptions.DEFAULT_QUERY_SERVER_DOAS_PARAM);
+ this.paramRemoteUserExtractor = new
HttpQueryStringParameterRemoteUserExtractor(doAsParam);
+ this.enableDoAs =
conf.getBoolean(QueryServices.QUERY_SERVER_DOAS_ENABLED_ATTRIB,
+ QueryServicesOptions.DEFAULT_QUERY_SERVER_DOAS_ENABLED);
+}
+
+@Override
+public String extract(HttpServletRequest request) throws
RemoteUserExtractionException {
+ if (request.getParameter(doAsParam) != null && enableDoAs) {
+String doAsUser = paramRemoteUserExtractor.extract(request);
+UserGroupInformation ugi =
UserGroupInformation.createRemoteUser(request.getRemoteUser());
+UserGroupInformation proxyUser =
UserGroupInformation.createProxyUser(doAsUser, ugi);
+
+// Check if this user is allowed to be impersonated.
+// Will throw AuthorizationException if the impersonation as this
user is not allowed
+try {
+ ProxyUsers.authorize(proxyUser, request.getRemoteAddr());
+ return doAsUser;
+} catch (AuthorizationException e) {
+ throw new RemoteUserExtractionException(e.getMessage());
--- End diff --
Can the exception be passed into the RemoteUserExtractionException instead
of just the message? (to preserve the stack trace)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #265: PHOENIX-3598
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/265#discussion_r124110879
--- Diff:
phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java
---
@@ -274,6 +282,47 @@ public int run(String[] args) throws Exception {
}
/**
+ * Use the correctly way to extract end user.
+ */
+
+ static class PhoenixRemoteUserExtractor implements RemoteUserExtractor{
+private final HttpQueryStringParameterRemoteUserExtractor
paramRemoteUserExtractor;
+private final HttpRequestRemoteUserExtractor
requestRemoteUserExtractor;
+private final boolean enableDoAs;
+private final String doAsParam;
+
+public PhoenixRemoteUserExtractor(Configuration conf) {
+ this.requestRemoteUserExtractor = new
HttpRequestRemoteUserExtractor();
+ this.doAsParam = conf.get(QueryServices.QUERY_SERVER_DOAS_PARAM,
+ QueryServicesOptions.DEFAULT_QUERY_SERVER_DOAS_PARAM);
+ this.paramRemoteUserExtractor = new
HttpQueryStringParameterRemoteUserExtractor(doAsParam);
+ this.enableDoAs =
conf.getBoolean(QueryServices.QUERY_SERVER_DOAS_ENABLED_ATTRIB,
+ QueryServicesOptions.DEFAULT_QUERY_SERVER_DOAS_ENABLED);
+}
+
+@Override
+public String extract(HttpServletRequest request) throws
RemoteUserExtractionException {
+ if (request.getParameter(doAsParam) != null && enableDoAs) {
--- End diff --
This can be simplified when we remove the `enableDoAs` logic.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #265: PHOENIX-3598
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/265#discussion_r124110721
--- Diff:
phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java
---
@@ -274,6 +282,47 @@ public int run(String[] args) throws Exception {
}
/**
+ * Use the correctly way to extract end user.
+ */
+
+ static class PhoenixRemoteUserExtractor implements RemoteUserExtractor{
+private final HttpQueryStringParameterRemoteUserExtractor
paramRemoteUserExtractor;
+private final HttpRequestRemoteUserExtractor
requestRemoteUserExtractor;
+private final boolean enableDoAs;
+private final String doAsParam;
+
+public PhoenixRemoteUserExtractor(Configuration conf) {
+ this.requestRemoteUserExtractor = new
HttpRequestRemoteUserExtractor();
+ this.doAsParam = conf.get(QueryServices.QUERY_SERVER_DOAS_PARAM,
+ QueryServicesOptions.DEFAULT_QUERY_SERVER_DOAS_PARAM);
+ this.paramRemoteUserExtractor = new
HttpQueryStringParameterRemoteUserExtractor(doAsParam);
+ this.enableDoAs =
conf.getBoolean(QueryServices.QUERY_SERVER_DOAS_ENABLED_ATTRIB,
--- End diff --
Can you move this check of whether or not we enable `doAs` above to
selectively call `withRemoteUserExtractor`, please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #265: PHOENIX-3598
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/265#discussion_r124113153 --- Diff: phoenix-queryserver/pom.xml --- @@ -147,6 +147,10 @@ commons-logging commons-logging + + org.mockito + mockito-all --- End diff -- Needs a `test` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/236#discussion_r123116460
--- Diff:
phoenix-load-balancer/src/main/java/org/apache/phoenix/loadbalancer/service/LoadBalancer.java
---
@@ -0,0 +1,170 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.loadbalancer.service;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Lists;
+import com.google.common.net.HostAndPort;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.curator.framework.CuratorFrameworkFactory;
+import org.apache.curator.framework.api.UnhandledErrorListener;
+import org.apache.curator.framework.recipes.cache.PathChildrenCache;
+import org.apache.curator.framework.state.ConnectionState;
+import org.apache.curator.framework.state.ConnectionStateListener;
+import org.apache.curator.retry.ExponentialBackoffRetry;
+import org.apache.curator.utils.CloseableUtils;
+import org.apache.hadoop.hbase.HBaseConfiguration;
+
+import java.net.ConnectException;
+import java.util.ArrayList;
+import java.util.concurrent.ThreadLocalRandom;
+import java.io.Closeable;
+import java.util.List;
+
+
+public class LoadBalancer {
+
+private static final LoadBalanceZookeeperConf CONFIG = new
LoadBalanceZookeeperConfImpl(HBaseConfiguration.create());
+private static CuratorFramework curaFramework = null;
+protected static final Log LOG = LogFactory.getLog(LoadBalancer.class);
+private static PathChildrenCache cache = null;
+private static final LoadBalancer loadBalancer = new LoadBalancer();
+private ConnectionStateListener connectionStateListener = null;
+private UnhandledErrorListener unhandledErrorListener = null;
+private List closeAbles = Lists.newArrayList();
+
+private LoadBalancer() {
+try {
+start();
+}catch(Exception ex){
+LOG.error("Exception while creating a zookeeper clients and
cache",ex);
+if ((curaFramework != null) && (connectionStateListener !=
null)){
+curaFramework.getConnectionStateListenable()
+.removeListener(connectionStateListener);
+}
+if ((curaFramework != null) && (unhandledErrorListener !=
null)){
+curaFramework.getUnhandledErrorListenable()
+.removeListener(unhandledErrorListener);
+}
+for (Closeable closeable : closeAbles) {
+CloseableUtils.closeQuietly(closeable);
+}
+}
+}
+
+
+
+/**
+ * Return Singleton Load Balancer every single time.
+ * @return LoadBalancer
+ */
+public static LoadBalancer getLoadBalancer() {
+return loadBalancer;
+}
+
+/**
+ * return the location of Phoenix Query Server
--- End diff --
Javadoc could use some updating to reflect the use of HostAndPort and alert
users that they will see an exception if there are no registered PQS instances.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/236#discussion_r123116500 --- Diff: phoenix-queryserver/src/it/resources/log4j.properties --- @@ -1,63 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -# Define some default values that can be overridden by system properties -hbase.root.logger=DEBUG,console --- End diff -- Did you accidentally delete this file..? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix issue #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/236 Ok, thanks for the pointer, Rahul. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/236#discussion_r122508226
--- Diff:
phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java
---
@@ -233,16 +240,29 @@ public int run(String[] args) throws Exception {
// Build and start the HttpServer
server = builder.build();
server.start();
+ registerToServiceProvider(hostname);
runningLatch.countDown();
server.join();
return 0;
} catch (Throwable t) {
LOG.fatal("Unrecoverable service error. Shutting down.", t);
this.t = t;
return -1;
+} finally {
+ deRegister();
}
}
+ private void registerToServiceProvider(String hostName) throws Exception
{
+ PqsZookeeperConf pqsZookeeperConf = new
PqsZookeeperConfImpl(getConf());
--- End diff --
Move any IT exercising loadbalancer functionality to the loadbalancer
module :)
Tests in the queryserver module would have no load balancer implementation.
PQS should pass gracefully in this case (e.g. not try to register with the
loadbalancer because it would be null).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/236#discussion_r122323098
--- Diff:
phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java
---
@@ -233,16 +240,29 @@ public int run(String[] args) throws Exception {
// Build and start the HttpServer
server = builder.build();
server.start();
+ registerToServiceProvider(hostname);
runningLatch.countDown();
server.join();
return 0;
} catch (Throwable t) {
LOG.fatal("Unrecoverable service error. Shutting down.", t);
this.t = t;
return -1;
+} finally {
+ deRegister();
}
}
+ private void registerToServiceProvider(String hostName) throws Exception
{
+ PqsZookeeperConf pqsZookeeperConf = new
PqsZookeeperConfImpl(getConf());
--- End diff --
Here's an example from Avatica on how it uses ServiceLoader to pull Metrics
backend systems:
A [factory interface is defined in one
module](https://github.com/apache/calcite-avatica/blob/master/metrics/src/main/java/org/apache/calcite/avatica/metrics/MetricsSystemFactory.java),
an [implementation is defined in a different
module](https://github.com/apache/calcite-avatica/blob/master/metrics-dropwizardmetrics3/src/main/java/org/apache/calcite/avatica/metrics/dropwizard3/DropwizardMetricsSystemFactory.java),
a [services file is created stating that the implementation is present for the
interface](https://github.com/apache/calcite-avatica/blob/master/metrics-dropwizardmetrics3/src/main/resources/META-INF/services/org.apache.calcite.avatica.metrics.MetricsSystemFactory),
the Avatica server [loads an implementation of the interface using
ServiceLoader](https://github.com/apache/calcite-avatica/blob/master/server/src/main/java/org/apache/calcite/avatica/server/HandlerFactory.java#L114)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/236#discussion_r122076328
--- Diff:
phoenix-queryserver/src/main/java/org/apache/phoenix/queryserver/server/QueryServer.java
---
@@ -233,16 +240,29 @@ public int run(String[] args) throws Exception {
// Build and start the HttpServer
server = builder.build();
server.start();
+ registerToServiceProvider(hostname);
runningLatch.countDown();
server.join();
return 0;
} catch (Throwable t) {
LOG.fatal("Unrecoverable service error. Shutting down.", t);
this.t = t;
return -1;
+} finally {
+ deRegister();
}
}
+ private void registerToServiceProvider(String hostName) throws Exception
{
+ PqsZookeeperConf pqsZookeeperConf = new
PqsZookeeperConfImpl(getConf());
--- End diff --
> implementation in load balancer module and interface def in query server
module
Correct.
> Then load the implementation in queryserver module via service loader
from loadbalancer module
Yup
> How do make query server dependent on load balancer module when load
balancer has a dependency on queryserver. Is there any plugin to load the jars
from loadbalancer into queryserver target
This is what ServiceLoader solves. Perhaps you do not understand how
ServiceLoader works? At compile time, PQS only knows about the interface. At
runtime, both the interface and the implementation are present. ServiceLoader
provides the implementation of the interface (based on the runtime classpath).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix issue #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on the issue: https://github.com/apache/phoenix/pull/236 Also, maybe I'm just not seeing it, but how does this load balancer get used by clients? It seems like this is only the advertisement component, and there is nothing for the thin client to use it. Is that intended? (in other words, you would do the rest later) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/236#discussion_r120396712
--- Diff:
phoenix-queryserver/src/main/java/org/apache/phoenix/loadbalancer/service/PqsZookeeperConf.java
---
@@ -0,0 +1,42 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.loadbalancer.service;
+
+import com.google.common.net.HostAndPort;
+import org.apache.zookeeper.data.ACL;
+
+import java.util.List;
+
+
+public interface PqsZookeeperConf {
--- End diff --
Nit: Suggest "LoadBalanceZookeeperConf" instead of "PqsZookeeperConf".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/236#discussion_r120395606
--- Diff: pom.xml ---
@@ -953,6 +961,26 @@
joda-time
${jodatime.version}
+
+org.apache.curator
+curator-framework
--- End diff --
This appears to be unused too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/236#discussion_r120395552
--- Diff: pom.xml ---
@@ -953,6 +961,26 @@
joda-time
${jodatime.version}
+
+org.apache.curator
+curator-framework
+${curator.version}
+
+
+org.apache.curator
+curator-x-discovery
--- End diff --
This appears to be unused.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/236#discussion_r120392825
--- Diff:
phoenix-queryserver/src/main/java/org/apache/phoenix/loadbalancer/service/PqsZookeeperConfImpl.java
---
@@ -0,0 +1,97 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.phoenix.loadbalancer.service;
+
+import com.google.common.net.HostAndPort;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.query.QueryServicesOptions;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Id;
+
+import java.util.Arrays;
+import java.util.List;
+
+
+
+public class PqsZookeeperConfImpl implements PqsZookeeperConf {
--- End diff --
This implementation should be in phoenix-loadbalancer, no?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #236: PHOENIX-3654 Load Balancer for thin client
Github user joshelser commented on a diff in the pull request: https://github.com/apache/phoenix/pull/236#discussion_r120394867 --- Diff: phoenix-queryserver/pom.xml --- @@ -147,6 +147,10 @@ commons-logging commons-logging + + org.apache.curator + curator-client --- End diff -- Assuming you follow my suggestion about using service loader, the curator dependencies can be removed from this pom.xml. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
