You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/08/10 18:51:05 UTC

[GitHub] [pinot] vvivekiyer opened a new issue, #9194: [BugFix] Fix bug with logging request headers.

vvivekiyer opened a new issue, #9194:
URL: https://github.com/apache/pinot/issues/9194

   In PR https://github.com/apache/pinot/pull/8907, `RequestIdentity` is assumed to be of type `HttpRequestIdentity`. Using `GrpcRequesterIdentity` will result in error like cannot be cast to class `org.apache.pinot.broker.api.HttpRequesterIdentity`.
   
   Potential fix is to have a function in `RequesterIdentity` called `getClientIp` that is implemented by all subclasses. 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] abhs50 commented on issue #9194: [BugFix] Fix bug with logging request headers.

Posted by GitBox <gi...@apache.org>.
abhs50 commented on issue #9194:
URL: https://github.com/apache/pinot/issues/9194#issuecomment-1212498928

   Yes I will look into this. 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] npawar closed issue #9194: [BugFix] Fix bug with logging request headers.

Posted by GitBox <gi...@apache.org>.
npawar closed issue #9194: [BugFix] Fix bug with logging request headers.
URL: https://github.com/apache/pinot/issues/9194


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] vvivekiyer commented on issue #9194: [BugFix] Fix bug with logging request headers.

Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on issue #9194:
URL: https://github.com/apache/pinot/issues/9194#issuecomment-1211126740

   @abhs50 would you be able to pick this up?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] abhs50 commented on issue #9194: [BugFix] Fix bug with logging request headers.

Posted by GitBox <gi...@apache.org>.
abhs50 commented on issue #9194:
URL: https://github.com/apache/pinot/issues/9194#issuecomment-1212610628

   Another option is we could also do below 
   `if (enableClientIpLogging && requesterIdentity instanceof HttpRequesterIdentity) `  on https://github.com/apache/pinot/blob/67344859c270b9e64638b6691d456e57a7570403/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java#L678


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] Jackie-Jiang commented on issue #9194: [BugFix] Fix bug with logging request headers.

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on issue #9194:
URL: https://github.com/apache/pinot/issues/9194#issuecomment-1211222897

   We may also add `getHttpHeaders()` to the interface, and by default returns an empty map


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] vvivekiyer commented on issue #9194: [BugFix] Fix bug with logging request headers.

Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on issue #9194:
URL: https://github.com/apache/pinot/issues/9194#issuecomment-1213525988

   IMO, I would avoid using `instanceOf` to achieve this. Here's the [reason](https://armedia.com/blog/instanceof-avoid-in-code/) for the same.
   
   What I was suggesting is as follows: 
   Have a method called `getClientIp` in RequesterIdentity. The default implementation can be to return "unknown"/empty. Allow all the subclasses to have their own implementation for `getClientIp`. The implementation of `getClientIp` in `HttpRequesterIdentity` would borrow the code from [here](https://github.com/apache/pinot/blob/67344859c270b9e64638b6691d456e57a7570403/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java#L679). 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] abhs50 commented on issue #9194: [BugFix] Fix bug with logging request headers.

Posted by GitBox <gi...@apache.org>.
abhs50 commented on issue #9194:
URL: https://github.com/apache/pinot/issues/9194#issuecomment-1212602484

   @vvivekiyer : Just to understand the design here, are you suggesting we move [https://github.com/apache/pinot/blob/67344859c270b9e64638b6691d456e57a7570403/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java#L671](url) inside a standalone method called getClientIp in org.apache.pinot.broker.api.HttpRequesterIdentity ?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org