[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/2639
  
Still +1. Thanks @arunmahadevan 


---

[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...

[arunmahadevan]

Github user arunmahadevan commented on the issue:

https://github.com/apache/storm/pull/2639
  
@ptgoetz , can you take a look again?


---

[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...

[arunmahadevan]

Github user arunmahadevan commented on the issue:

https://github.com/apache/storm/pull/2639
  
@ptgoetz , thanks for the suggestion.

Yes, it was clear why the distributed flag was needed when it was not used 
anywhere else in the code. I was hoping that the user will set the right 
parallelism while consuming from topics. I can add it back.


---

[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/2639
  
Looks good to me. +1

I was wondering why you removed the `distributed` flag, and realized it was 
never properly implemented! For it to work, we would need the following method 
override:

```java
@Override
public Map getComponentConfiguration() {
if(!_isDistributed) {
Map ret = new HashMap();
ret.put(Config.TOPOLOGY_MAX_TASK_PARALLELISM, 1);
return ret;
} else {
return null;
}
}
```

I can see cases where that flag would be useful, for example when 
connecting to a topic vs. a queue. We may want to leave the flag there and fix 
the override.


---

[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...

[arunmahadevan]

Github user arunmahadevan commented on the issue:

https://github.com/apache/storm/pull/2639
  
@HeartSaVioR , I made some changes so that the order of the methods does 
not matter and the final validation happens in "open". I also ran the example 
topology and things look fine.


---

[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...

[arunmahadevan]

Github user arunmahadevan commented on the issue:

https://github.com/apache/storm/pull/2639
  
@HeartSaVioR , please check it again. 


---

[GitHub] storm issue #2639: STORM-3035: fix the issue in JmsSpout.ack when toCommit i...

[arunmahadevan]

Github user arunmahadevan commented on the issue:

https://github.com/apache/storm/pull/2639
  
This started out as a fix to handle the exceptions in "ack" when toCommit 
was empty. However during the review process and testing, figured out many more 
issues with the current approach. Also some JMS providers like Tibco supports 
ACK ing individual messages, which could not be handled with the existing code. 
The async mode of consuming the messages was also problematic to ensure 
at-least once delivery even with locks/synchronization since ack-ing an 
individual JMS message in CLIENT_ACK mode was going to ack the messages 
received in the listener (even if the listener did not return).

To handle all the issues, I have refactored quite and bit and changed the 
approach of consuming the messages from async (listener based) to sync 
(receive) and introduced MessageHandlers to handle the emit/ack/fail in 
different ways based on the mode.

@HeartSaVioR , can you review it again and let me know what you think?


---