[
https://issues.apache.org/jira/browse/STORM-3925?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17735140#comment-17735140
]
Bharat Gulati edited comment on STORM-3925 at 6/26/23 9:37 AM:
---------------------------------------------------------------
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.-
Above is true only for newer versions when the relevant classes were translated
from clojure to java. In older versions trying to do the same will lead to the
following exception:
{code:java}
2023-06-26 14:49:57.916 o.a.s.d.worker main [ERROR] Error on initialization of
server mk-worker
java.lang.UnsupportedOperationException: null
at clojure.lang.APersistentMap.put(APersistentMap.java:374)
~[clojure-1.7.0.jar:?] {code}
*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.
was (Author: JIRAUSER300951):
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
> Time Spent: 40m
> Remaining Estimate: 0h
>
> 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)
