[jira] [Commented] (STORM-3925) Allow user resources (in WorkerTopologyContext) to be set by Worker Hooks

["Bharat Gulati (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/STORM-3925?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17735140#comment-17735140 ] 

Bharat Gulati commented on STORM-3925:
--------------------------------------

Current Interfaces / classes:

 
{code:java}
public interface IWorkerHook extends Serializable {
    /**
     * This method is called when a worker is started.
     *
     * @param topoConf The Storm configuration for this worker
     * @param context  This object can be used to get information about this worker's place within the topology
     */
    void start(Map<String, Object> topoConf, WorkerTopologyContext context);

    /**
     * This method is called right before a worker shuts down.
     */
    void shutdown();
}


public class TopologyContext extends WorkerTopologyContext implements IMetricsContext {
...
}

public class WorkerTopologyContext extends GeneralTopologyContext {
...
} {code}
 

 

Approaches Evaluated:

*A. Leave things as it is:*

Currently there exists a work-around for users wherein they can set the shared resources inside topoConf.put(...) or via context.getConf().put(...) in the worker hook and can retrieve the shared resource back inside Tasks via the same.

Pros: the desired functionality is already available and works.
Cons: the behaviour is an implementation side-effect and not intended and may break later and reliance upon it will be promoting bad practice.

*B. Expose a new IWorkerHook interface:*

The IWorkerHook interface used the WorkerTopologyContext directly. Since this class is a super-class of TopologyContext, we can't restrict any new public method introduced in WorkerTopologyContext from appearing in TopologyContext and hence a user will be able to call setResource from both WorkerHook and Tasks. To correct that, we can re-implement IWorkerHook with a V2 version and deprecate the previous one gradually.

Pros: allows us to introduce proper interface from scratch
Cons: unnecessary maintenance overhead

*C. Expose setResource to both WorkerHook and Tasks with proper documentation:*

As stated in B since there is no way to prevent a public function added in WorkerTopologyContext from appearing in TopologyContext, we will allow that to happen and add relevant comments to suggest users how to use it but won't be able to prevent its incorrect usage. A slight variation here can be to override setResource inside TopologyContext to be a no-op or throw some NotAllowException (though I believe both the latter options are actually worse since they break intuitiveness)

Pros: keeps the change simple
Cons: relies upon users to read the documentation and use them properly.

*D. Sub-class WorkerTopologyContext and extend the IWorkerHook interface to add another start method using that sub-class: (*g)*

This intends to create a sub-class of WorkerTopologyContext and expose the relevant setResource method there. Additionally, by adding another start method to the IWorkerHook which exposes the new sub-class instead of WorkerTopologyContext, only WorkerHook will be able to invoke setResource. Additionally, the previous start method (and new start method) in IWorkerHook will become default methods to allow for back-ward compatibility. (v2.x drops the support for Java7, so this should be fine from that angle too: https://storm.apache.org/2019/05/30/storm200-released.html)

Pros: keeps the change simple
Cons: we are still exposing concrete classes via IWorkerHook instead of an interface.

 

Based on above, I suggest going ahead with Option D, let me know your thoughts on the same. I will submit a PR accordingly.

> Allow user resources (in WorkerTopologyContext) to be set by Worker Hooks
> -------------------------------------------------------------------------
>
>                 Key: STORM-3925
>                 URL: https://issues.apache.org/jira/browse/STORM-3925
>             Project: Apache Storm
>          Issue Type: Improvement
>          Components: storm-core
>            Reporter: Bharat Gulati
>            Priority: Minor
>
> The current implementation of WorkerTopologyContext in WorkerState will always lead to empty userResources as no interface exposes a way to allow user to set them.
> {code:java}
>     private Map<String, Object> 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<>();
>     } {code}
> The intention will be to expose the relevant methods under a separate class which can then allow users to set the resources from WorkerHooks while only providing get access from Tasks (i.e. via TopologyContext)



--
This message was sent by Atlassian Jira
(v8.20.10#820010)