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

[via GitHub]

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

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

[PR Review] Support for declaring WorkerHook in Flux topology definitions

[6harat]

Hey,

I submitted a PR for review yesterday and am looking for some help from
existing contributors regarding the below:

1. new JIRA ticket for the feature (and if someone can also approve my JIRA
account creation request if they are one of the approvers)
2. trigger the github workflow: https://github.com/apache/storm/pull/3546
(I already ran the test locally on my machine)
3. provide PR review comments

(attached the related mail from users mailing list)

Regards
6harat
--- Begin Message ---
I have opened the following PR: https://github.com/apache/storm/pull/3546
regarding the same. Please review the same.

Few pointers where I would need help from the community:
1. I don't have an Apache JIRA account yet. Already raised the request a
couple of days ago, it seems to be still pending approval. As a result I
don't have a corresponding JIRA ticket available for this feature which can
be tagged to the pull request and where such discussions can be taken up.
2. I went through the DEVELOPER.md and it mentions: *"See _Development
workflow_ above on how you can create a pull request, for instance."*
However, I was not able to find the corresponding Development workflow
section. Hence, I have prepared the PR based on what I observed in the
previous merged PRs. Let me know if anything needs to be changed or please
point me to the right section where these details are already documented.
3. I am also looking to make this feature available in older versions
starting from 1.x.x till latest. Let me know what is the procedure for
that. AFAIS, the flux core got repackaged between this time frame and
perhaps I may have to create a separate branch to take care of that for
older versions.
4. What will be required from my side to publish the new jar versions for
flux-core (and other impacted artifacts)

Regards
Bharat Gulati

On Fri, Jun 16, 2023 at 1:46 PM 6harat 
wrote:

> Sure, will look to submit a draft PR this weekend.
>
> On Thu, 15 Jun 2023, 12:20 Richard Zowalla,  wrote:
>
>> Hey,
>>
>> I think, that it would be a good addition.
>> If you have some spare time, I guess, that a PR would be very welcome.
>>
>> Gruß
>> Richard
>>
>>
>> Am 15. Juni 2023 08:16:14 MESZ schrieb 6harat <
>> bharat.gulati.ce...@gmail.com>:
>>
>>> Hey,
>>>
>>> A while back the support for declaring WorkerHook in TopologyBuilder was
>>> added. Related JIRA: https://issues.apache.org/jira/browse/STORM-126
>>>
>>> However, this feature has still not made it to FluxBuilder. Wanted to
>>> check if there are plans to bring parity in this regard.
>>>
>>> Seems like a small change only, happy to submit a PR if the feature is
>>> accepted.
>>>
>>> Regards
>>> 6harat
>>>
>>>

-- 
6harat
solr enthusiast
[not affiliated to core dev team]
--- End Message ---