[GitHub] storm pull request #2843: STORM-3230: Add in sync if key not found
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2843#discussion_r218922619
--- Diff:
storm-client/src/jvm/org/apache/storm/security/auth/workertoken/WorkerTokenAuthorizer.java
---
@@ -95,9 +95,12 @@ private static IStormClusterState
buildStateIfNeeded(Map conf, T
throw new IllegalArgumentException("Token is not valid, token
has expired.");
}
-PrivateWorkerKey key = keyCache.getUnchecked(deser);
-if (key == null) {
-throw new IllegalArgumentException("Token is not valid,
private key not found.");
+PrivateWorkerKey key;
+try {
+key = keyCache.getUnchecked(deser);
+} catch (CacheLoader.InvalidCacheLoadException e) {
+//This happens when the cache has a null returned to it.
--- End diff --
I'll update the comment.
---
[GitHub] storm pull request #2841: STORM-3105: upgrade version of hive
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2841 ---
[GitHub] storm pull request #2840: STORM-3147: Add metrics based on ClusterSummary
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2840 ---
[GitHub] storm pull request #2843: STORM-3230: Add in sync if key not found
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2843#discussion_r218905963
--- Diff:
storm-client/src/jvm/org/apache/storm/security/auth/workertoken/WorkerTokenAuthorizer.java
---
@@ -95,9 +95,12 @@ private static IStormClusterState
buildStateIfNeeded(Map conf, T
throw new IllegalArgumentException("Token is not valid, token
has expired.");
}
-PrivateWorkerKey key = keyCache.getUnchecked(deser);
-if (key == null) {
-throw new IllegalArgumentException("Token is not valid,
private key not found.");
+PrivateWorkerKey key;
+try {
+key = keyCache.getUnchecked(deser);
+} catch (CacheLoader.InvalidCacheLoadException e) {
+//This happens when the cache has a null returned to it.
--- End diff --
not sure about this. If `getUnchecked(deser);` returns null, it will throw
an exception?
---
[GitHub] storm pull request #2843: STORM-3230: Add in sync if key not found
Github user agresch commented on a diff in the pull request:
https://github.com/apache/storm/pull/2843#discussion_r218902320
--- Diff:
storm-client/src/jvm/org/apache/storm/cluster/StormClusterStateImpl.java ---
@@ -903,6 +911,7 @@ public void removeAllPrivateWorkerKeys(String
topologyId) {
for (WorkerTokenServiceType type :
WorkerTokenServiceType.values()) {
String path = ClusterUtils.secretKeysPath(type, topologyId);
try {
+LOG.debug("Removing worker keys under {}", path);
--- End diff --
If this does not hit often, I would vote for making this info.
---
[GitHub] storm issue #2840: STORM-3147: Add metrics based on ClusterSummary
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2840 I worked with @zd-project fairly closely to come up with the goals for the metrics. I just think we can do better on some of the metrics, but it is not something we have to do right now. I am +1 for merging this in as is, but I think we will need some follow on JIRA to clean things up and document them better. ---
[GitHub] storm pull request #2843: STORM-3230: Add in sync if key not found
GitHub user revans2 opened a pull request: https://github.com/apache/storm/pull/2843 STORM-3230: Add in sync if key not found You can merge this pull request into a Git repository by running: $ git pull https://github.com/revans2/incubator-storm STORM-3230 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2843.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 #2843 commit 839071367754338b83096e9d1f908f859e9d5493 Author: Robert (Bobby) Evans Date: 2018-09-19T15:05:23Z STORM-3230: Add in sync if key not found ---
Re: Regarding releasing Apache Storm 2.0.0
Sounds good. I just filed https://issues.apache.org/jira/browse/STORM-3230 and I'll be putting up a pull request shortly. I would like to see it in before a 2.x release, but it is kind of minor because ZK has to really be overloaded to hit this, and we tend to recover after a while. I'll look at getting the rest in ASAP. Thanks, Bobby On Tue, Sep 18, 2018 at 3:54 PM P. Taylor Goetz wrote: > I’m ready to release when everything is ready to go. Since we haven’t > released from the 2.0-based master branch, I wouldn’t be surprised if I run > into release issues, but I’ll slog through it. > > -Taylor > > > On Sep 18, 2018, at 10:46 AM, Bobby Evans wrote: > > > > Great work everyone. We are really close on this. We have everything in > > except for https://github.com/apache/storm/pull/2719, but there has > been no > > movement there, so I will try and put up an alternative pull request. > > > > Also We noticed that a recent merge broke some things fairly badly so we > > need to get https://github.com/apache/storm/pull/2839 in, but that is > just > > a matter of waiting a few more hours for the 24 hours to be up. > > > > Great work everyone, hopefully we will have an RC up for a vote a little > > over a day from now. > > > > Thanks, > > > > Bobby > > > > P.S. Taylor, You have put up all of the release candidates in the past > and > > done all of the votes for them. If you want to continue the trend that > is > > fine with me, but if not I am happy to do it, but I might have to bug you > > to be sure I do it all correctly. > > > > On Mon, Sep 17, 2018 at 9:13 AM Bobby Evans wrote: > > > >> I think we are really close on this and I would love to see us get an RC > >> out ASAP. > >> > >> We are still missing some things that Stig called out. > >> > >> https://github.com/apache/storm/pull/2719 has a build issue, not sure > if > >> we need to make an alternative patch or not. > >> https://github.com/apache/storm/pull/2800 has a newer alternative > patch > >> https://github.com/apache/storm/pull/2836 please take a look. > >> https://github.com/apache/storm/pull/2805 has some merge conflicts > >> currently, but everyone please take a chance to review it. > >> > >> Thanks, > >> > >> Bobby > >> > >> On Fri, Sep 14, 2018 at 2:57 AM Jungtaek Lim wrote: > >> > >>> I have sought the name of client artifact from some of streaming > >>> frameworks. Please refer below: > >>> > >>> Spark: spark-core > >>> Kafka: kafka-clients > >>> Flink: flink-clients > >>> Heron: heron-api > >>> > >>> Based on divergence, I don't see the reason "storm-core" is the only > name > >>> which avoid confusion. Actually, if my understanding is right, we need > to > >>> let end users including "storm-server" when running local cluster, then > >>> "storm-core" vs "storm-server" would give real confusion. I guess we > >>> already discussed about the naming, and given that we don't rename it > we > >>> are OK with renamed artifacts. > >>> > >>> 2018년 9월 14일 (금) 오후 4:07, Roshan Naik >님이 > >>> 작성: > >>> > Happy to see consensus in moving fwd with 2.0 soon. > I will try to get a minor patch (STORM-3205) within 24 hours ... as it > seems like it has potential to deliver a decent perf boost and energy > savings. > One thing I am hoping we can address before releasing Storm 2 is... to > >>> fix > the naming of the storm-client.jar. Its such a core jar really, it > >>> should > have been really called storm-core or something like that... but > unfortunately we already have another jar with that name. Retaining > the > 'client' name for this new jar would be confusing and give wrong > impressions to users and any new devs IMO. > -roshan > > On Thursday, September 13, 2018, 2:12:40 PM PDT, Govind Menon > wrote: > > STORM-3217 and STORM-3221 have been fixed - +1 from me for 2.0 RC. > > On Wed, Sep 12, 2018 at 10:01 AM Govind Menon > wrote: > > > Hi all, > > > > There are some regressions that I introduced as part of STORM-1311 > >>> which > > I'm working on as part of > https://issues.apache.org/jira/browse/STORM-3217 > > and https://issues.apache.org/jira/browse/STORM-3221. These should > be > > fixed before a 2.x release > > > > I have code working on the Yahoo internal branch and should have PRs > >>> up > > for them in community soon. > > > > I apologize for slowing things up. > > > > Thanks, > > Govind. > > > > On Tue, Sep 11, 2018 at 3:31 PM Arun Mahadevan > >>> wrote: > > > >> +1 for releasing 2.0. > >> > >> May be the RC can be cut once critical patches are merged. > >> > >> On Tue, 11 Sep 2018 at 10:28, Stig Rohde Døssing < > stigdoess...@gmail.com> > >> wrote: > >> > >>> +1 to cut an RC. > >>> > >>> Here are a couple of PRs that could maybe go in > >>> > >>> https://github.com/apache/storm/pull/2719 > >>>
[GitHub] storm issue #2842: STORM-3229: Add in better error reporting
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2842 @danny0405 I changed the log to info. That is a great point, because the only time you would see that under normally would be if someone had configured the cluster to use digest auth, which we really only have around for testing purposes. ---
[GitHub] storm pull request #2842: STORM-3229: Add in better error reporting
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2842#discussion_r218691752
--- Diff:
storm-client/src/jvm/org/apache/storm/security/auth/workertoken/WorkerTokenAuthorizer.java
---
@@ -112,13 +112,21 @@ private static IStormClusterState
buildStateIfNeeded(Map conf, T
if (keyCache == null) {
return Optional.empty();
}
+byte[] user = null;
+WorkerTokenInfo deser = null;
+try {
+user = Base64.getDecoder().decode(userName);
+deser = Utils.deserialize(user, WorkerTokenInfo.class);
+} catch (Exception e) {
+LOG.debug("Could not decode {}, might just be a plain digest
request...", userName, e);
+return Optional.empty();
--- End diff --
Could we change to LOG.INFO if the request it not that frequentï¼we can
see more details from the log.
---
