Github user d2r commented on the pull request:

    https://github.com/apache/storm/pull/521#issuecomment-103145088
  
    @HeartSaVioR,
    
    I see you are right.  There are two race conditions here:
    
    1. If we do not check assignments in the read-lock, then we could use an 
invalid connection. There could be an exception if we lose this race.
    
    2. If we group by destination node+port when we equeue tuples for sending, 
we lose the information needed to update the destination node+port when we 
dequeue for sending, since assignments `task->node+port` can change in the 
meantime.  If we lose this race, then tuples for which there is a new 
assignment may be dropped unnecessarily (if we fix 1.) or sent to the wrong 
worker.
    
    (Formerly, I did not realize 2. was also a regression with 861a92e, so I 
was looking for a smaller change in this PR.)
    
    > If you think latter is better to move on, I'll post a new PR based on 
6ef2f11.
    
    OK, I will take a look at your other branch.
    
    >  IMO it is a critical path for transfer efficiency, so it would be better 
to have more reviewer for this patch.
    
    I agree. The more, the better.


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