You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by sudheeshkatkam <gi...@git.apache.org> on 2016/05/04 08:13:22 UTC
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
GitHub user sudheeshkatkam opened a pull request:
https://github.com/apache/drill/pull/495
DRILL-4654: Add new metrics to the MetricRegistry
+ New metrics: running queries, pending queries, completed queries,
used memory (root allocator)
+ Borrow SystemPropertyUtil class from Netty
+ Configure DrillMetrics params through system properties
+ Deprecate getMetrics method in contextual objects
+ Rename "current" to "used" for RPC allocator current memory usage to
follow convention
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sudheeshkatkam/drill DRILL-4654
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/495.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #495
----
commit 1fc205bebaf3c1f20df85996ba49cf98b60e4c9e
Author: Sudheesh Katkam <sk...@maprtech.com>
Date: 2016-05-04T06:10:47Z
DRILL-4654: Add new metrics to the MetricRegistry
+ New metrics: running queries, pending queries, completed queries,
used memory (root allocator)
+ Borrow SystemPropertyUtil class from Netty
+ Configure DrillMetrics params through system properties
+ Deprecate getMetrics method in contextual objects
+ Rename "current" to "used" for RPC allocator current memory usage to
follow convention
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/495#issuecomment-217221522
Good points. I added a comment in code, but it does not completely address your concerns.
Unfortunately, not all class (e.g. RootAllocator) can be instrumented for metrics if the single instance is part of a contextual object. Static instance is fine according to [library doc](https://dropwizard.github.io/metrics/3.1.0/getting-started/#the-registry). This means maintaining state in a static object. Is there a workaround?
AFAIK we do not provide a way to run multiple drillbits in the same JVM in production use cases. You are right, metrics will conflict for unit tests (as is, right now).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on the pull request:
https://github.com/apache/drill/pull/495#issuecomment-216937718
We currently use multiple drillbits in the same JVM process for our unit tests, won't this cause them to conflict with one another? I don't know if production use cases try to run multiple Drillbits in a single JVM, but even if this isn't a use case, a stateful API like the metrics registry seems like the wrong thing to move more towards static shared API. While I know it is currently static, I think the idea with these methods you were deprecating is that we would try to move to not be accessing anything static.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on the pull request:
https://github.com/apache/drill/pull/495#issuecomment-219774989
+1
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/495#discussion_r63544498
--- Diff: common/src/main/java/org/apache/drill/exec/util/SystemPropertyUtil.java ---
@@ -0,0 +1,189 @@
+/**
+ * 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.drill.exec.util;
+
+import java.security.AccessController;
+import java.security.PrivilegedAction;
+import java.util.regex.Pattern;
+
+/**
+ * A collection of utility methods to retrieve and parse the values of Java system properties.
+ *
+ * This is a modified version of Netty's internal system property utility class.
+ */
+public final class SystemPropertyUtil {
+ private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SystemPropertyUtil.class);
+
+ private static final Pattern INTEGER_PATTERN = Pattern.compile("-?[0-9]+");
+
+ /**
+ * Returns {@code true} if and only if the system property with the specified {@code key}
+ * exists.
+ */
+ public static boolean contains(String key) {
+ return get(key) != null;
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to {@code null} if the property access fails.
+ *
+ * @return the property value or {@code null}
+ */
+ public static String get(String key) {
+ return get(key, null);
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to the specified default value if
+ * the property access fails.
+ *
+ * @return the property value.
+ * {@code def} if there's no such property or if an access to the
+ * specified property is not allowed.
+ */
+ public static String get(final String key, String def) {
+ if (key == null) {
+ throw new NullPointerException("key");
+ }
+ if (key.isEmpty()) {
+ throw new IllegalArgumentException("key must not be empty.");
+ }
+
+ String value = null;
+ try {
+ if (System.getSecurityManager() == null) {
+ value = System.getProperty(key);
+ } else {
+ value = AccessController.doPrivileged(new PrivilegedAction<String>() {
+ @Override
+ public String run() {
+ return System.getProperty(key);
+ }
+ });
+ }
+ } catch (Exception e) {
+ logger.warn("Unable to retrieve a system property '" + key + "'; default values will be used.", e);
+ }
+
+ if (value == null) {
+ return def;
+ }
+
+ return value;
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to the specified default value if
+ * the property access fails.
+ *
+ * @return the property value.
+ * {@code def} if there's no such property or if an access to the
+ * specified property is not allowed.
+ */
+ public static boolean getBoolean(String key, boolean def) {
+ String value = get(key);
+ if (value == null) {
+ return def;
+ }
+
+ value = value.trim().toLowerCase();
+ if (value.isEmpty()) {
+ return true;
+ }
+
+ if ("true".equals(value) || "yes".equals(value) || "1".equals(value)) {
+ return true;
+ }
+
+ if ("false".equals(value) || "no".equals(value) || "0".equals(value)) {
+ return false;
+ }
+
+ logger.warn("Unable to parse the boolean system property '{}':{} - using the default value: {}",
+ key, value, def);
+
+ return def;
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to the specified default value if
+ * the property access fails.
+ *
+ * @return the property value.
+ * {@code def} if there's no such property or if an access to the
+ * specified property is not allowed.
+ */
+ public static int getInt(String key, int def) {
+ String value = get(key);
+ if (value == null) {
+ return def;
+ }
+
+ value = value.trim().toLowerCase();
+ if (INTEGER_PATTERN.matcher(value).matches()) {
+ try {
+ return Integer.parseInt(value);
+ } catch (Exception e) {
+ // Ignore
+ }
+ }
+
+ logger.warn("Unable to parse the integer system property '{}':{} - using the default value: {}",
+ key, value, def);
+
+ return def;
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to the specified default value if
+ * the property access fails.
+ *
+ * @return the property value.
+ * {@code def} if there's no such property or if an access to the
+ * specified property is not allowed.
+ */
+ public static long getLong(String key, long def) {
+ String value = get(key);
+ if (value == null) {
+ return def;
+ }
+
+ value = value.trim().toLowerCase();
+ if (INTEGER_PATTERN.matcher(value).matches()) {
--- End diff --
same here
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/495
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/495#issuecomment-219757831
Some minor comments, otherwise +1 LGTM
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/495#discussion_r63546312
--- Diff: common/src/main/java/org/apache/drill/exec/util/SystemPropertyUtil.java ---
@@ -0,0 +1,189 @@
+/**
+ * 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.drill.exec.util;
+
+import java.security.AccessController;
+import java.security.PrivilegedAction;
+import java.util.regex.Pattern;
+
+/**
+ * A collection of utility methods to retrieve and parse the values of Java system properties.
+ *
+ * This is a modified version of Netty's internal system property utility class.
--- End diff --
What is the motivation behind re-implementing this utility class ? I'm too lazy to check what's the difference between netty's implementation and this one =P
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/495#discussion_r63593914
--- Diff: common/src/main/java/org/apache/drill/exec/util/SystemPropertyUtil.java ---
@@ -0,0 +1,189 @@
+/**
+ * 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.drill.exec.util;
+
+import java.security.AccessController;
+import java.security.PrivilegedAction;
+import java.util.regex.Pattern;
+
+/**
+ * A collection of utility methods to retrieve and parse the values of Java system properties.
+ *
+ * This is a modified version of Netty's internal system property utility class.
--- End diff --
The class was internal to Netty. So I made a copy and changed the logger.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/495#discussion_r63544441
--- Diff: common/src/main/java/org/apache/drill/exec/util/SystemPropertyUtil.java ---
@@ -0,0 +1,189 @@
+/**
+ * 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.drill.exec.util;
+
+import java.security.AccessController;
+import java.security.PrivilegedAction;
+import java.util.regex.Pattern;
+
+/**
+ * A collection of utility methods to retrieve and parse the values of Java system properties.
+ *
+ * This is a modified version of Netty's internal system property utility class.
+ */
+public final class SystemPropertyUtil {
+ private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SystemPropertyUtil.class);
+
+ private static final Pattern INTEGER_PATTERN = Pattern.compile("-?[0-9]+");
+
+ /**
+ * Returns {@code true} if and only if the system property with the specified {@code key}
+ * exists.
+ */
+ public static boolean contains(String key) {
+ return get(key) != null;
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to {@code null} if the property access fails.
+ *
+ * @return the property value or {@code null}
+ */
+ public static String get(String key) {
+ return get(key, null);
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to the specified default value if
+ * the property access fails.
+ *
+ * @return the property value.
+ * {@code def} if there's no such property or if an access to the
+ * specified property is not allowed.
+ */
+ public static String get(final String key, String def) {
+ if (key == null) {
+ throw new NullPointerException("key");
+ }
+ if (key.isEmpty()) {
+ throw new IllegalArgumentException("key must not be empty.");
+ }
+
+ String value = null;
+ try {
+ if (System.getSecurityManager() == null) {
+ value = System.getProperty(key);
+ } else {
+ value = AccessController.doPrivileged(new PrivilegedAction<String>() {
+ @Override
+ public String run() {
+ return System.getProperty(key);
+ }
+ });
+ }
+ } catch (Exception e) {
+ logger.warn("Unable to retrieve a system property '" + key + "'; default values will be used.", e);
+ }
+
+ if (value == null) {
+ return def;
+ }
+
+ return value;
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to the specified default value if
+ * the property access fails.
+ *
+ * @return the property value.
+ * {@code def} if there's no such property or if an access to the
+ * specified property is not allowed.
+ */
+ public static boolean getBoolean(String key, boolean def) {
+ String value = get(key);
+ if (value == null) {
+ return def;
+ }
+
+ value = value.trim().toLowerCase();
+ if (value.isEmpty()) {
+ return true;
+ }
+
+ if ("true".equals(value) || "yes".equals(value) || "1".equals(value)) {
+ return true;
+ }
+
+ if ("false".equals(value) || "no".equals(value) || "0".equals(value)) {
+ return false;
+ }
+
+ logger.warn("Unable to parse the boolean system property '{}':{} - using the default value: {}",
+ key, value, def);
+
+ return def;
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to the specified default value if
+ * the property access fails.
+ *
+ * @return the property value.
+ * {@code def} if there's no such property or if an access to the
+ * specified property is not allowed.
+ */
+ public static int getInt(String key, int def) {
+ String value = get(key);
+ if (value == null) {
+ return def;
+ }
+
+ value = value.trim().toLowerCase();
+ if (INTEGER_PATTERN.matcher(value).matches()) {
--- End diff --
We don't really need this pattern matcher, if value is not a proper integer `Integer.parseInt` will throw an exception and we already handle that case.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/495#discussion_r63597486
--- Diff: common/src/main/java/org/apache/drill/exec/util/SystemPropertyUtil.java ---
@@ -0,0 +1,189 @@
+/**
+ * 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.drill.exec.util;
+
+import java.security.AccessController;
+import java.security.PrivilegedAction;
+import java.util.regex.Pattern;
+
+/**
+ * A collection of utility methods to retrieve and parse the values of Java system properties.
+ *
+ * This is a modified version of Netty's internal system property utility class.
+ */
+public final class SystemPropertyUtil {
+ private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SystemPropertyUtil.class);
+
+ private static final Pattern INTEGER_PATTERN = Pattern.compile("-?[0-9]+");
+
+ /**
+ * Returns {@code true} if and only if the system property with the specified {@code key}
+ * exists.
+ */
+ public static boolean contains(String key) {
+ return get(key) != null;
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to {@code null} if the property access fails.
+ *
+ * @return the property value or {@code null}
+ */
+ public static String get(String key) {
+ return get(key, null);
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to the specified default value if
+ * the property access fails.
+ *
+ * @return the property value.
+ * {@code def} if there's no such property or if an access to the
+ * specified property is not allowed.
+ */
+ public static String get(final String key, String def) {
+ if (key == null) {
+ throw new NullPointerException("key");
+ }
+ if (key.isEmpty()) {
+ throw new IllegalArgumentException("key must not be empty.");
+ }
+
+ String value = null;
+ try {
+ if (System.getSecurityManager() == null) {
+ value = System.getProperty(key);
+ } else {
+ value = AccessController.doPrivileged(new PrivilegedAction<String>() {
+ @Override
+ public String run() {
+ return System.getProperty(key);
+ }
+ });
+ }
+ } catch (Exception e) {
+ logger.warn("Unable to retrieve a system property '" + key + "'; default values will be used.", e);
+ }
+
+ if (value == null) {
+ return def;
+ }
+
+ return value;
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to the specified default value if
+ * the property access fails.
+ *
+ * @return the property value.
+ * {@code def} if there's no such property or if an access to the
+ * specified property is not allowed.
+ */
+ public static boolean getBoolean(String key, boolean def) {
+ String value = get(key);
+ if (value == null) {
+ return def;
+ }
+
+ value = value.trim().toLowerCase();
+ if (value.isEmpty()) {
+ return true;
+ }
+
+ if ("true".equals(value) || "yes".equals(value) || "1".equals(value)) {
+ return true;
+ }
+
+ if ("false".equals(value) || "no".equals(value) || "0".equals(value)) {
+ return false;
+ }
+
+ logger.warn("Unable to parse the boolean system property '{}':{} - using the default value: {}",
+ key, value, def);
+
+ return def;
+ }
+
+ /**
+ * Returns the value of the Java system property with the specified
+ * {@code key}, while falling back to the specified default value if
+ * the property access fails.
+ *
+ * @return the property value.
+ * {@code def} if there's no such property or if an access to the
+ * specified property is not allowed.
+ */
+ public static int getInt(String key, int def) {
+ String value = get(key);
+ if (value == null) {
+ return def;
+ }
+
+ value = value.trim().toLowerCase();
+ if (INTEGER_PATTERN.matcher(value).matches()) {
--- End diff --
I am not sure why the pattern matcher was in place. The difference I see is "+1729" is allowed by `Integer.parseInt(...)` but not this matcher.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/495#issuecomment-219192360
@jaltekruse I updated the PR (reverted deprecation); please review?
The commit message has details.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---