You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by agresch <gi...@git.apache.org> on 2018/02/05 21:00:25 UTC
[GitHub] storm pull request #2548: STORM-2935 Add metric to track when TGT expires
GitHub user agresch opened a pull request:
https://github.com/apache/storm/pull/2548
STORM-2935 Add metric to track when TGT expires
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/agresch/storm agresch_athens
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2548.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 #2548
----
commit 49ab628a952a58c236ad31d93a939ab75794e131
Author: Aaron Gresch <ag...@...>
Date: 2018-02-05T20:56:13Z
Add metric to track when TGT expires
----
---
[GitHub] storm pull request #2548: STORM-2935 Add metric to track when TGT expires
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on a diff in the pull request:
https://github.com/apache/storm/pull/2548#discussion_r166117240
--- Diff: storm-client/src/jvm/org/apache/storm/daemon/worker/WorkerState.java ---
@@ -20,6 +20,17 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Sets;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.*;
--- End diff --
That was just a re-order.
---
[GitHub] storm pull request #2548: STORM-2935 Add metric to track when TGT expires
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2548#discussion_r166111464
--- Diff: storm-client/src/jvm/org/apache/storm/daemon/worker/WorkerState.java ---
@@ -20,6 +20,17 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Sets;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.*;
--- End diff --
I believe storm community says better to not use *.
Btw, in Intellij, you can navigate Preference--> Editor --> code style --> Java and set `class count to use import with *` to a high number to avoid auto collapse
---
[GitHub] storm pull request #2548: STORM-2935 Add metric to track when TGT expires
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2548#discussion_r166435892
--- Diff: storm-client/src/jvm/org/apache/storm/executor/bolt/BoltExecutor.java ---
@@ -77,6 +83,17 @@ public void init(Map<Integer, Task> idToTask) {
Map cachedNodePortToSocket = (Map) workerData.getCachedNodeToPortSocket().get();
BuiltinMetricsUtil.registerIconnectionClientMetrics(cachedNodePortToSocket, topoConf, userContext);
BuiltinMetricsUtil.registerIconnectionServerMetric(workerData.getReceiver(), topoConf, userContext);
+
+ // add any autocredential expiry metrics from the worker
+ if (workerData.getAutoCredentials() != null) {
+ int bucketSize = ((Number) topoConf.get(Config.TOPOLOGY_BUILTIN_METRICS_BUCKET_SIZE_SECS)).intValue();
+ for (IAutoCredentials autoCredential : workerData.getAutoCredentials()) {
+ if (autoCredential instanceof IMetric) {
+ IMetric metric = (IMetric)autoCredential;
+ userContext.registerMetric(metric.getMetricName(), metric, bucketSize);
--- End diff --
`metric.getMetricName()` makes sense for only this case. I agree that's easiest way to address, but how about having new interface like `RegisteringMetrics` or another better name which has `registerMetrics(TopologyContext topoContext, Map<String, Object> topoConf)` registering metrics by itself?
This will require only change of AutoTGT when migrating Metrics V1 into Metrics V2, and also enable registering multiple metrics if needed. (You can't register multiple metrics in this way in Metrics V2, whereas there's workaround to provide Map from `getValueAndReset()` like what other built-in metrics are doing in Metrics V1.)
---
[GitHub] storm pull request #2548: STORM-2935 Add metric to track when TGT expires
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2548#discussion_r166201854
--- Diff: storm-client/src/jvm/org/apache/storm/daemon/worker/WorkerState.java ---
@@ -20,6 +20,17 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Sets;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.*;
--- End diff --
Might be better to fix once we found it, but this is a kind of nit.
---
[GitHub] storm pull request #2548: STORM-2935 Add metric to track when TGT expires
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2548
---
[GitHub] storm issue #2548: STORM-2935 Add metric to track when TGT expires
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on the issue:
https://github.com/apache/storm/pull/2548
We're trying to bring this change back to older releases internally. Since v2 is not available and we want to backport this, could we get this in and address changing to v2 in the near future?
---
[GitHub] storm issue #2548: STORM-2935 Add metric to track when TGT expires
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2548
@agresch
I meant having metric field in AutoTGT so that it can be changed to Gauge in Codahale metric for Metrics V2, but it's OK if lambda works in Metrics V1.
---
[GitHub] storm issue #2548: STORM-2935 Add metric to track when TGT expires
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2548
Looks like this patch proposes making a change to Metrics V1 interface which is deprecated. I'd like to see us having a plan to migrate Metrics V1 into Metrics V2 instead of keep using it.
---
[GitHub] storm issue #2548: STORM-2935 Add metric to track when TGT expires
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on the issue:
https://github.com/apache/storm/pull/2548
@HeartSaVioR - is this what you were requesting?
---
[GitHub] storm issue #2548: STORM-2935 Add metric to track when TGT expires
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2548
I just noticed that Metrics V2 is not port-forwarded to master branch. I'd suggest waiting for Metrics V2 integration against master, and apply it unless it is urgent.
---
[GitHub] storm pull request #2548: STORM-2935 Add metric to track when TGT expires
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2548#discussion_r166460108
--- Diff: storm-client/src/jvm/org/apache/storm/security/auth/kerberos/AutoTGT.java ---
@@ -33,18 +33,21 @@
import javax.security.auth.login.LoginContext;
import javax.xml.bind.DatatypeConverter;
+import org.apache.storm.Config;
import org.apache.storm.metric.api.IMetric;
+import org.apache.storm.metric.api.IMetricsRegistrant;
import org.apache.storm.security.auth.AuthUtils;
import org.apache.storm.security.auth.IAutoCredentials;
import org.apache.storm.security.auth.ICredentialsRenewer;
+import org.apache.storm.task.TopologyContext;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Automatically take a user's TGT, and push it, and renew it in Nimbus.
*/
-public class AutoTGT implements IAutoCredentials, ICredentialsRenewer, IMetric {
+public class AutoTGT implements IAutoCredentials, ICredentialsRenewer, IMetric, IMetricsRegistrant {
--- End diff --
We can have individual metric instance and get rid of needs for implementing `IMetric`. It should be removed in Metrics V2 anyway.
---
[GitHub] storm issue #2548: STORM-2935 Add metric to track when TGT expires
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on the issue:
https://github.com/apache/storm/pull/2548
@HeartSaVioR Let me know if this looks better.
---