You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2020/01/31 18:14:04 UTC

[GitHub] [knox] pzampino opened a new pull request #250: KNOX-2209 - Improve logging for Knox token handling

pzampino opened a new pull request #250: KNOX-2209 - Improve logging for Knox token handling
URL: https://github.com/apache/knox/pull/250
 
 
   ## What changes were proposed in this pull request?
   
   Added logging to the Token and TokenState services.
   
   ## How was this patch tested?
   
   Manually tested by way of visual inspection of logs.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [knox] pzampino commented on a change in pull request #250: KNOX-2209 - Improve logging for Knox token handling

Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #250: KNOX-2209 - Improve logging for Knox token handling
URL: https://github.com/apache/knox/pull/250#discussion_r373628555
 
 

 ##########
 File path: gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java
 ##########
 @@ -384,4 +391,9 @@ private long getExpiry() {
     }
     return expiry;
   }
+
+  private String getTopologyName() {
+    return (String) context.getAttribute("org.apache.knox.gateway.gateway.cluster");
 
 Review comment:
   +1

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [knox] risdenk commented on a change in pull request #250: KNOX-2209 - Improve logging for Knox token handling

Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #250: KNOX-2209 - Improve logging for Knox token handling
URL: https://github.com/apache/knox/pull/250#discussion_r373615249
 
 

 ##########
 File path: gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenServiceMessages.java
 ##########
 @@ -22,7 +22,7 @@
 import org.apache.knox.gateway.i18n.messages.Messages;
 import org.apache.knox.gateway.i18n.messages.StackTrace;
 
-@Messages(logger="org.apache.knox.gateway.service.knoxsso")
+@Messages(logger="org.apache.knox.gateway.service.token")
 
 Review comment:
   Might make sense to match the package name? `knoxtoken`

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [knox] pzampino merged pull request #250: KNOX-2209 - Improve logging for Knox token handling

Posted by GitBox <gi...@apache.org>.
pzampino merged pull request #250: KNOX-2209 - Improve logging for Knox token handling
URL: https://github.com/apache/knox/pull/250
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [knox] risdenk commented on a change in pull request #250: KNOX-2209 - Improve logging for Knox token handling

Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #250: KNOX-2209 - Improve logging for Knox token handling
URL: https://github.com/apache/knox/pull/250#discussion_r373615072
 
 

 ##########
 File path: gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java
 ##########
 @@ -384,4 +391,9 @@ private long getExpiry() {
     }
     return expiry;
   }
+
+  private String getTopologyName() {
+    return (String) context.getAttribute("org.apache.knox.gateway.gateway.cluster");
 
 Review comment:
   nit: Would be good to make this a constant. I ran into this looking into something else. It was NOT clear that this attribute was the topology name.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [knox] pzampino commented on a change in pull request #250: KNOX-2209 - Improve logging for Knox token handling

Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #250: KNOX-2209 - Improve logging for Knox token handling
URL: https://github.com/apache/knox/pull/250#discussion_r373620041
 
 

 ##########
 File path: gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenServiceMessages.java
 ##########
 @@ -22,7 +22,7 @@
 import org.apache.knox.gateway.i18n.messages.Messages;
 import org.apache.knox.gateway.i18n.messages.StackTrace;
 
-@Messages(logger="org.apache.knox.gateway.service.knoxsso")
+@Messages(logger="org.apache.knox.gateway.service.token")
 
 Review comment:
   I can buy that. I was surprised to see that it had been knoxsso :-O

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [knox] pzampino commented on a change in pull request #250: KNOX-2209 - Improve logging for Knox token handling

Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #250: KNOX-2209 - Improve logging for Knox token handling
URL: https://github.com/apache/knox/pull/250#discussion_r373619768
 
 

 ##########
 File path: gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java
 ##########
 @@ -384,4 +391,9 @@ private long getExpiry() {
     }
     return expiry;
   }
+
+  private String getTopologyName() {
+    return (String) context.getAttribute("org.apache.knox.gateway.gateway.cluster");
 
 Review comment:
   This is why I put it in a method that makes it more clear (aside from the obvious benefit of re-use). I can create a constant for it. Are you suggesting a constant in this class, or somewhere else for broader consumption?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [knox] risdenk commented on a change in pull request #250: KNOX-2209 - Improve logging for Knox token handling

Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #250: KNOX-2209 - Improve logging for Knox token handling
URL: https://github.com/apache/knox/pull/250#discussion_r373621258
 
 

 ##########
 File path: gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java
 ##########
 @@ -384,4 +391,9 @@ private long getExpiry() {
     }
     return expiry;
   }
+
+  private String getTopologyName() {
+    return (String) context.getAttribute("org.apache.knox.gateway.gateway.cluster");
 
 Review comment:
   Broader consumption - doesn't have to be as part of this change. The string `"org.apache.knox.gateway.gateway.cluster"` is being used to insert the topology value into the context. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services