[GitHub] [storm] 6harat commented on pull request #3546: [STORM-3924] Support for declaring WorkerHook in Flux topology definitions

2023-08-04 Thread via GitHub 


6harat commented on PR #3546:
URL: https://github.com/apache/storm/pull/3546#issuecomment-1665297205

   will rebase and check it out over the weekend


-- 
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: dev-unsubscr...@storm.apache.org

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

[GitHub] [storm] 6harat commented on pull request #3546: [STORM-3924] Support for declaring WorkerHook in Flux topology definitions

2023-07-23 Thread via GitHub 


6harat commented on PR #3546:
URL: https://github.com/apache/storm/pull/3546#issuecomment-1647188305

   @bipinprasad just pushed a README update commit, the prev workflow was 
already successful, so triggering it again might not be reqd. Please let me 
know if anything else is required from my side for going ahead with the merge.


-- 
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: dev-unsubscr...@storm.apache.org

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

[GitHub] [storm] 6harat commented on pull request #3546: [STORM-3924] Support for declaring WorkerHook in Flux topology definitions

2023-06-25 Thread via GitHub 


6harat commented on PR #3546:
URL: https://github.com/apache/storm/pull/3546#issuecomment-1605932509

   > I was going through other parts of the code and found this TODO. Seems 
like it was intended to be connected to WorkerHooks when those were first 
introduced.
   > 
   > ```
   > private Map makeUserResources() {
   > /* TODO: need to invoke a hook provided by the topology, giving it 
a chance to create user resources.
   >  * this would be part of the initialization hook
   >  * need to separate workertopologycontext into WorkerContext and 
WorkerUserContext.
   >  * actually just do it via interfaces. just need to make sure to 
hide setResource from tasks
   >  */
   > return new HashMap<>();
   > }
   > ```
   > 
   > AFAIS, the current implementation will always lead to empty userResources 
as no interface exposes a way to allow user to set them.
   > 
   > Lmk, if my above understanding is correct and this change can also be 
taken up. this would allow for better application context management. Will put 
this fix in a separate PR.
   > 
   > cc: @abhishekagarwal87 , @schonfeld , @revans2 , @nathanmarz , @rzo1 , 
@bipinprasad
   
   this has been taken up here: https://github.com/apache/storm/pull/3547


-- 
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: dev-unsubscr...@storm.apache.org

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