Camel
  1. Camel
  2. CAMEL-3065

Netty - @deprecated sync option and let it use the MEP to decide if its InOnly or InOut

    Details

    • Type: Improvement Improvement
    • Status: In Progress
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.4.0
    • Fix Version/s: 3.0.0
    • Component/s: camel-mina, camel-netty
    • Labels:
      None

      Activity

      Hide
      Claus Ibsen added a comment -

      The consumer needs to have a sync option.

      So we may want to rename it to sendReply=true|false so its more obvious what it does.
      Then the producer side uses the MEP of the Exchange to decide if it should wait for a reply or not. So the option only apply for the consumer.

      Show
      Claus Ibsen added a comment - The consumer needs to have a sync option. So we may want to rename it to sendReply=true|false so its more obvious what it does. Then the producer side uses the MEP of the Exchange to decide if it should wait for a reply or not. So the option only apply for the consumer.
      Hide
      Claus Ibsen added a comment -

      However the sync option was inspired from the camel-mina component which has this option.

      Show
      Claus Ibsen added a comment - However the sync option was inspired from the camel-mina component which has this option.
      Hide
      Ashwin Karpe added a comment - - edited

      Hi Claus,

      I have fixed this rather straightforward issue and attached a diff file for your review.

      It is a very simple change for changing sync to sendReply along with its getter/setter. I did have to fix a whole lot of tests along with some netty classes that looked up sync to use sendReply instead of sync.

      Can you please let me know what you think. I will check it in following your +1.

      Cheers,

      Ashwin...

      Show
      Ashwin Karpe added a comment - - edited Hi Claus, I have fixed this rather straightforward issue and attached a diff file for your review. It is a very simple change for changing sync to sendReply along with its getter/setter. I did have to fix a whole lot of tests along with some netty classes that looked up sync to use sendReply instead of sync. Can you please let me know what you think. I will check it in following your +1. Cheers, Ashwin...
      Hide
      Hadrian Zbarcea added a comment - - edited

      @Ashwin,
      Thanks for the patch, it is indeed straightforward. Two comments though:
      1. i think sendReply is equally confusing (in a different way) and my preference would be inOut. Since this is what happens anyway, the exchange will be InOut, it is imho clearer what the semantics of this option is. Or, if you want, you can have it as a String instead of boolean and call it mep (better then just pattern I guess, with possible values inOnly or inOut with the former being the default).
      2. You are now a committer, so please free to commit the code unless you have doubts in which case attaching a patch or adding a link to a git branch is ok. I doubt you had doubts in this case .

      One more thing. I am not sure if this is necessary, camel-netty being fairly new, but if you want to just deprecate @sync, you can mark isSync() and setSync as @deprecated and have them get/set inOut (assuming you will use that instead of sendReply).

      Show
      Hadrian Zbarcea added a comment - - edited @Ashwin, Thanks for the patch, it is indeed straightforward. Two comments though: 1. i think sendReply is equally confusing (in a different way) and my preference would be inOut . Since this is what happens anyway, the exchange will be InOut, it is imho clearer what the semantics of this option is. Or, if you want, you can have it as a String instead of boolean and call it mep (better then just pattern I guess, with possible values inOnly or inOut with the former being the default). 2. You are now a committer, so please free to commit the code unless you have doubts in which case attaching a patch or adding a link to a git branch is ok. I doubt you had doubts in this case . One more thing. I am not sure if this is necessary, camel-netty being fairly new, but if you want to just deprecate @sync, you can mark isSync() and setSync as @deprecated and have them get/set inOut (assuming you will use that instead of sendReply ).
      Hide
      Claus Ibsen added a comment -

      Actually I think we should change this for Camel 3.0, to keep the old behavior in the 2.x.
      Because both camel-netty and camel-mina have the sync option.

      In Camel 3.0 we can change that to check the MEP of the Exchange being InOut and then automatic expect a reply.
      Hence I would like to wait with this for Camel 3.0, and also update camel-mina as well.

      Show
      Claus Ibsen added a comment - Actually I think we should change this for Camel 3.0, to keep the old behavior in the 2.x. Because both camel-netty and camel-mina have the sync option. In Camel 3.0 we can change that to check the MEP of the Exchange being InOut and then automatic expect a reply. Hence I would like to wait with this for Camel 3.0, and also update camel-mina as well.
      Hide
      Ashwin Karpe added a comment -

      Hi Hadrian & Claus,

      I will change the code to use inOut instead of sendReply. I will attach and commit a fresh patch.

      Claus, would you rather this fix go in 3.x or would you prefer that I mark sync as deprecated and introduce a new inOut as an option. This will not break anybody in 2.x but will create the necessary capability for future adopters.

      Please let me know.

      Cheers,

      Ashwin...

      Show
      Ashwin Karpe added a comment - Hi Hadrian & Claus, I will change the code to use inOut instead of sendReply. I will attach and commit a fresh patch. Claus, would you rather this fix go in 3.x or would you prefer that I mark sync as deprecated and introduce a new inOut as an option. This will not break anybody in 2.x but will create the necessary capability for future adopters. Please let me know. Cheers, Ashwin...
      Hide
      Claus Ibsen added a comment -

      Ashwin the patch will affect current logic, as they don't consider the MEP at all.

      So we should wait until Camel 3.0 to change this. Then we can alter both camel-netty and camel-mina to use the new behavior.

      Show
      Claus Ibsen added a comment - Ashwin the patch will affect current logic, as they don't consider the MEP at all. So we should wait until Camel 3.0 to change this. Then we can alter both camel-netty and camel-mina to use the new behavior.
      Hide
      Claus Ibsen added a comment -

      Do the same fix on camel-mina as well, when implement this ticket.

      Show
      Claus Ibsen added a comment - Do the same fix on camel-mina as well, when implement this ticket.
      Hide
      Ashwin Karpe added a comment -

      Sure Claus.

      I will make this fix on Netty and Mina in version 3.0.

      Cheers,

      Ashwin...

      Show
      Ashwin Karpe added a comment - Sure Claus. I will make this fix on Netty and Mina in version 3.0. Cheers, Ashwin...

        People

        • Assignee:
          Ashwin Karpe
          Reporter:
          Claus Ibsen
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:

            Development