Github user HeartSaVioR commented on a diff in the pull request:

    https://github.com/apache/storm/pull/2241#discussion_r129519731
  
    --- Diff: storm-client/src/jvm/org/apache/storm/daemon/Task.java ---
    @@ -177,6 +190,38 @@ public BuiltinMetrics getBuiltInMetrics() {
             return builtInMetrics;
         }
     
    +    // TODO: ROSHAN:  There appears to be a bug here... this msg should go 
to 'this' task... not based on outgoing tasks
    +    public void sendUnanchored(String stream, List<Object> values, 
ExecutorTransfer transfer) {
    --- End diff --
    
    I'm curious the reason of moving this and below methods are moved to this 
class.
    
    If we leave this method to Executor class, `transfer` parameter can be 
removed since caller passes ExecutorTransfer in Executor itself for all the 
cases. 
    
    So which is more natural? 
    - moving this method to Task class and keeping `ExecutorTransfer` parameter
    - leaving this method to Executor class as it is and removing 
`ExecutorTransfer` parameter
    
    I prefer not exposing field in instance if not necessary. The 
implementations of OutputCollector are also using `getExecutorTransfer()` and I 
also would like to add transfer() method and hide it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to