Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.6.0
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      I've started a new component to interface with Amazon Web Services. This first pass includes just a Simple Queue Service component. Additional services will be added soon. I used the Amazon AWS SDK for Java to interface with AWS. Uses Apache 2.0 as it's license.

      Let me know if I need to change things or any thoughts/suggestions and I'll be glad to make the adjustments.

      1. patchfile.txt
        35 kB
        Tracy Snell
      2. CAMEL-3468.patch
        55 kB
        Christian Müller

        Issue Links

          Activity

          Hide
          Tracy Snell added a comment -

          Once this code is in trunk I'll create the component documentation for the web site.

          Show
          Tracy Snell added a comment - Once this code is in trunk I'll create the component documentation for the web site.
          Hide
          Mark Ford added a comment -

          I created one for the SNS back in May 2010. It's up on google code. Perhaps the two should merge?

          http://code.google.com/p/camel-sns/

          Show
          Mark Ford added a comment - I created one for the SNS back in May 2010. It's up on google code. Perhaps the two should merge? http://code.google.com/p/camel-sns/
          Hide
          Tracy Snell added a comment -

          Mark that would rock! I'll merge them up if you don't mind.

          Show
          Tracy Snell added a comment - Mark that would rock! I'll merge them up if you don't mind.
          Hide
          Tracy Snell added a comment -

          I debated a good bit if this should be camel-sqs or part of a larger camel-aws. I went with the same route as camel-gae and opted for camel-aws. Still almost 50/50 on which is better.

          Show
          Tracy Snell added a comment - I debated a good bit if this should be camel-sqs or part of a larger camel-aws. I went with the same route as camel-gae and opted for camel-aws. Still almost 50/50 on which is better.
          Hide
          Mark Ford added a comment -

          You're welcome to take as much or as little of the code from my camel-sns component. I was reluctant to contribute it earlier since the tests require an AWS account and I wasn't sure how this would work during regular builds. Looking back on the code now I should have split the tests into unit tests and integration tests.

          I've only setup subscribing to an SNS topic with a SQS endpoint. I didn't see a lot of value from the camel perspective of supporting the email/http/https delivery endpoints but perhaps others will disagree. As far as I remember, the tests were working and everything documented on the google code site is functional.

          Show
          Mark Ford added a comment - You're welcome to take as much or as little of the code from my camel-sns component. I was reluctant to contribute it earlier since the tests require an AWS account and I wasn't sure how this would work during regular builds. Looking back on the code now I should have split the tests into unit tests and integration tests. I've only setup subscribing to an SNS topic with a SQS endpoint. I didn't see a lot of value from the camel perspective of supporting the email/http/https delivery endpoints but perhaps others will disagree. As far as I remember, the tests were working and everything documented on the google code site is functional.
          Hide
          Tracy Snell added a comment -

          Christian, let me know what's missing to make this as easy as possible to add. First time I've tried to write a component so not positive I covered everything. I did get a clean mvn -Psourcecheck clean install
          but not sure what all I need. The unit testing relies on a mock since connecting to AWS requires user creds. Thought about adding a profile with a test that used a live queue. Devs would have to enter their AWS creds then make sure to delete them. Bailed on that and decided a simple example in the examples dir may be better and have less of a risk of someones creds getting posted in a patch or to svn.

          Show
          Tracy Snell added a comment - Christian, let me know what's missing to make this as easy as possible to add. First time I've tried to write a component so not positive I covered everything. I did get a clean mvn -Psourcecheck clean install but not sure what all I need. The unit testing relies on a mock since connecting to AWS requires user creds. Thought about adding a profile with a test that used a live queue. Devs would have to enter their AWS creds then make sure to delete them. Bailed on that and decided a simple example in the examples dir may be better and have less of a risk of someones creds getting posted in a patch or to svn.
          Hide
          Christian Müller added a comment -

          Hello Tracy,
          I will have a look on it tomorrow or at the weekend. Running

          mvn -Psourcecheck clean install
          

          is always a good idea . I will also have a look on the test coverage and, if it's free and simple to get an account by amazon, I will also run a integration test. I wonder this integration test could be part of the test suite, but annotated with

          @Test
          @Ignore("Must be manually tested")
          

          like the SmppComponentIntegrationTest. A separate example for this component is also a good idea, but requires more work (a README.txt, may be a wiki page like these [1], ...). Would you also like to work on it? And maybe improve this example over the time if we add more features to this component?

          By the way, I will also document the procedure of adding new components to Camel: CAMEL-3470

          [1] http://camel.apache.org/examples.html

          Show
          Christian Müller added a comment - Hello Tracy, I will have a look on it tomorrow or at the weekend. Running mvn -Psourcecheck clean install is always a good idea . I will also have a look on the test coverage and, if it's free and simple to get an account by amazon, I will also run a integration test. I wonder this integration test could be part of the test suite, but annotated with @Test @Ignore( "Must be manually tested" ) like the SmppComponentIntegrationTest. A separate example for this component is also a good idea, but requires more work (a README.txt, may be a wiki page like these [1] , ...). Would you also like to work on it? And maybe improve this example over the time if we add more features to this component? By the way, I will also document the procedure of adding new components to Camel: CAMEL-3470 [1] http://camel.apache.org/examples.html
          Hide
          Christian Müller added a comment -

          Hello Tracy,
          today I had a first look at this new component. It looks good and I think a cool contribution. However, I will make some little changes.
          I also made an integration test to verify this component works as expected. At present it fails, but I think i get it working the next few days, hopefully before Hadrian cut the 2.6. release...

          public class SqsComponentIntegrationTest extends CamelTestSupport {
              
              @EndpointInject(uri = "direct:start")
              private ProducerTemplate template;
              
              @EndpointInject(uri = "mock:result")
              private MockEndpoint result;
              
              @Test
              //@Ignore("Must be manually tested")
              public void sendInOnly() throws Exception {
                  result.expectedMessageCount(1);
                  
                  Exchange exchange = template.send("direct:start", ExchangePattern.InOnly, new Processor() {
                      public void process(Exchange exchange) throws Exception {
                          exchange.getIn().setBody("This is my message text.");
                      }
                  });
                  
                  assertMockEndpointsSatisfied();
                  
                  Exchange resultExchange = result.getExchanges().get(0);
                  assertEquals("This is my message text.", resultExchange.getIn().getBody());
                  assertNotNull(resultExchange.getIn().getHeader(SqsBinding.MESSAGE_ID));
                  assertNotNull(resultExchange.getIn().getHeader(SqsBinding.RECEIPT_HANDLE));
                  assertEquals("6a1559560f67c5e7a7d5d838bf0272ee", resultExchange.getIn().getHeader(SqsBinding.MD5_OF_BODY));
                  
                  assertNotNull(exchange.getIn().getHeader(SqsBinding.MESSAGE_ID));
                  assertEquals("6a1559560f67c5e7a7d5d838bf0272ee", resultExchange.getIn().getHeader(SqsBinding.MD5_OF_BODY));
              }
              
              @Test
              //@Ignore("Must be manually tested")
              public void sendInOut() throws Exception {
                  result.expectedMessageCount(1);
                  
                  Exchange exchange = template.send("direct:start", ExchangePattern.InOut, new Processor() {
                      public void process(Exchange exchange) throws Exception {
                          exchange.getIn().setBody("This is my message text.");
                      }
                  });
                  
                  assertMockEndpointsSatisfied();
                  
                  Exchange resultExchange = result.getExchanges().get(0);
                  assertEquals("This is my message text.", resultExchange.getIn().getBody());
                  assertNotNull(resultExchange.getIn().getHeader(SqsBinding.RECEIPT_HANDLE));
                  assertNotNull(resultExchange.getIn().getHeader(SqsBinding.MESSAGE_ID));
                  assertEquals("6a1559560f67c5e7a7d5d838bf0272ee", resultExchange.getIn().getHeader(SqsBinding.MD5_OF_BODY));
                  
                  assertNotNull(exchange.getOut().getHeader(SqsBinding.MESSAGE_ID));
                  assertEquals("6a1559560f67c5e7a7d5d838bf0272ee", resultExchange.getOut().getHeader(SqsBinding.MD5_OF_BODY));
              }
              
              protected RouteBuilder createRouteBuilder() throws Exception {
                  return new RouteBuilder() {
                      @Override
                      public void configure() throws Exception {
                          from("direct:start")
                              .to("sqs://MyQueue?accessKey=xxx&secretKey=yyy");
                          
                          from("sqs://MyQueue?accessKey=xxx&secretKey=yyy")
                              .to("mock:result");
                      }
                  };
              }
          }
          
          Show
          Christian Müller added a comment - Hello Tracy, today I had a first look at this new component. It looks good and I think a cool contribution. However, I will make some little changes. I also made an integration test to verify this component works as expected. At present it fails, but I think i get it working the next few days, hopefully before Hadrian cut the 2.6. release... public class SqsComponentIntegrationTest extends CamelTestSupport { @EndpointInject(uri = "direct:start" ) private ProducerTemplate template; @EndpointInject(uri = "mock:result" ) private MockEndpoint result; @Test //@Ignore( "Must be manually tested" ) public void sendInOnly() throws Exception { result.expectedMessageCount(1); Exchange exchange = template.send( "direct:start" , ExchangePattern.InOnly, new Processor() { public void process(Exchange exchange) throws Exception { exchange.getIn().setBody( "This is my message text." ); } }); assertMockEndpointsSatisfied(); Exchange resultExchange = result.getExchanges().get(0); assertEquals( "This is my message text." , resultExchange.getIn().getBody()); assertNotNull(resultExchange.getIn().getHeader(SqsBinding.MESSAGE_ID)); assertNotNull(resultExchange.getIn().getHeader(SqsBinding.RECEIPT_HANDLE)); assertEquals( "6a1559560f67c5e7a7d5d838bf0272ee" , resultExchange.getIn().getHeader(SqsBinding.MD5_OF_BODY)); assertNotNull(exchange.getIn().getHeader(SqsBinding.MESSAGE_ID)); assertEquals( "6a1559560f67c5e7a7d5d838bf0272ee" , resultExchange.getIn().getHeader(SqsBinding.MD5_OF_BODY)); } @Test //@Ignore( "Must be manually tested" ) public void sendInOut() throws Exception { result.expectedMessageCount(1); Exchange exchange = template.send( "direct:start" , ExchangePattern.InOut, new Processor() { public void process(Exchange exchange) throws Exception { exchange.getIn().setBody( "This is my message text." ); } }); assertMockEndpointsSatisfied(); Exchange resultExchange = result.getExchanges().get(0); assertEquals( "This is my message text." , resultExchange.getIn().getBody()); assertNotNull(resultExchange.getIn().getHeader(SqsBinding.RECEIPT_HANDLE)); assertNotNull(resultExchange.getIn().getHeader(SqsBinding.MESSAGE_ID)); assertEquals( "6a1559560f67c5e7a7d5d838bf0272ee" , resultExchange.getIn().getHeader(SqsBinding.MD5_OF_BODY)); assertNotNull(exchange.getOut().getHeader(SqsBinding.MESSAGE_ID)); assertEquals( "6a1559560f67c5e7a7d5d838bf0272ee" , resultExchange.getOut().getHeader(SqsBinding.MD5_OF_BODY)); } protected RouteBuilder createRouteBuilder() throws Exception { return new RouteBuilder() { @Override public void configure() throws Exception { from( "direct:start" ) .to( "sqs: //MyQueue?accessKey=xxx&secretKey=yyy" ); from( "sqs: //MyQueue?accessKey=xxx&secretKey=yyy" ) .to( "mock:result" ); } }; } }
          Hide
          Christian Müller added a comment -

          Mark,
          I had also a look on your component and I would also like to add your component into the camel-aws component. Could you please open a new JIRA for it and attach your source code? Don't forget to grant the Apache license so we can use it.
          Thanks in advance,
          Christian

          Show
          Christian Müller added a comment - Mark, I had also a look on your component and I would also like to add your component into the camel-aws component. Could you please open a new JIRA for it and attach your source code? Don't forget to grant the Apache license so we can use it. Thanks in advance, Christian
          Hide
          Tracy Snell added a comment -

          MessageID, MD5 and RecipientHandle are not part of the message attributes so weren't propagating. Adding them now. Can I see SqsBinding?

          Show
          Tracy Snell added a comment - MessageID, MD5 and RecipientHandle are not part of the message attributes so weren't propagating. Adding them now. Can I see SqsBinding?
          Hide
          Tracy Snell added a comment -

          Just need to add these 3 lines (no diff since it's not in svn yet) in the SqsConsumer poll method:

                      Map<String, Object> attributes = new HashMap<String, Object>(msg.getAttributes());
          
                      // Add message attributes to the header map
                      attributes.put(SqsBinding.MESSAGE_ID, msg.getMessageId());
                      attributes.put(SqsBinding.RECEIPT_HANDLE, msg.getReceiptHandle());
                      attributes.put(SqsBinding.MD5_OF_BODY, msg.getMD5OfBody());
          
                      message.setHeaders(attributes);
          

          That runs cleanly for everything but the last 2 lines of sendInOut()

          assertNotNull(exchange.getOut().getHeader("messageId"));
          assertEquals("6a1559560f67c5e7a7d5d838bf0272ee", resultExchange.getOut().getHeader("mD5OfBody"));
          

          Also you have exchange in the first line and resultExchange in the second (doesn't matter since out isn't set in either exhange). Looking at the out issue now.

          Show
          Tracy Snell added a comment - Just need to add these 3 lines (no diff since it's not in svn yet) in the SqsConsumer poll method: Map< String , Object > attributes = new HashMap< String , Object >(msg.getAttributes()); // Add message attributes to the header map attributes.put(SqsBinding.MESSAGE_ID, msg.getMessageId()); attributes.put(SqsBinding.RECEIPT_HANDLE, msg.getReceiptHandle()); attributes.put(SqsBinding.MD5_OF_BODY, msg.getMD5OfBody()); message.setHeaders(attributes); That runs cleanly for everything but the last 2 lines of sendInOut() assertNotNull(exchange.getOut().getHeader( "messageId" )); assertEquals( "6a1559560f67c5e7a7d5d838bf0272ee" , resultExchange.getOut().getHeader( "mD5OfBody" )); Also you have exchange in the first line and resultExchange in the second (doesn't matter since out isn't set in either exhange). Looking at the out issue now.
          Hide
          Claus Ibsen added a comment -

          Lets see if we can get this in Camel 2.6, if not postpone it to 2.7

          Show
          Claus Ibsen added a comment - Lets see if we can get this in Camel 2.6, if not postpone it to 2.7
          Hide
          Christian Müller added a comment -

          We need an OSGI-fied version of the aws-java-sdk. Normally the ServiceMix guys will provide one for us.

          Show
          Christian Müller added a comment - We need an OSGI-fied version of the aws-java-sdk. Normally the ServiceMix guys will provide one for us.
          Hide
          Christian Müller added a comment -

          @Claus: I think we should postpone it to 2.7, because we also need the OSGI-fied version of the aws-java-sdk.

          @Tracy: The resultExchange in the last line was a typo.
          The SqsBinding only holds the SQS constants. Maybe we find a better name for it...

          public class SqsBinding {
          
              public static final String MD5_OF_BODY = "CamelSqsMD5OfBody";
              public static final String MESSAGE_ID = "CamelSqsMessageId";
              public static final String RECEIPT_HANDLE = "CamelSqsReceiptHandle";
          }
          

          The integration test is now working fine. I would like to add/change the following thinks:

          • deleting of consumed messages after the successful processing in Camel
          • provide the option to configure a client in the URL like "sqs://MyQueue?accessKey=xxx&secretKey=yyy&client=#myAmazonSQSClient" to have the possibility to share and mock the client
          • provide the option to configure a header filter strategy. All not filtered header will be send as message attributes to SQS
          • add some log statements
          • add some "real" Camel tests which use an AmazonSQSClient mock
          Show
          Christian Müller added a comment - @Claus: I think we should postpone it to 2.7, because we also need the OSGI-fied version of the aws-java-sdk. @Tracy: The resultExchange in the last line was a typo. The SqsBinding only holds the SQS constants. Maybe we find a better name for it... public class SqsBinding { public static final String MD5_OF_BODY = "CamelSqsMD5OfBody" ; public static final String MESSAGE_ID = "CamelSqsMessageId" ; public static final String RECEIPT_HANDLE = "CamelSqsReceiptHandle" ; } The integration test is now working fine. I would like to add/change the following thinks: deleting of consumed messages after the successful processing in Camel provide the option to configure a client in the URL like "sqs://MyQueue?accessKey=xxx&secretKey=yyy&client=#myAmazonSQSClient" to have the possibility to share and mock the client provide the option to configure a header filter strategy. All not filtered header will be send as message attributes to SQS add some log statements add some "real" Camel tests which use an AmazonSQSClient mock
          Hide
          Tracy Snell added a comment -

          I don't understand this item:

          • provide the option to configure a client in the URL like "sqs://MyQueue?accessKey=xxx&secretKey=yyy&client=#myAmazonSQSClient" to have the possibility to share and mock the client

          What is myAmazonSQSClient?

          Side note, should all the class names be Sqs... or SQS...? I like the latter actually.

          Show
          Tracy Snell added a comment - I don't understand this item: provide the option to configure a client in the URL like "sqs://MyQueue?accessKey=xxx&secretKey=yyy&client=#myAmazonSQSClient" to have the possibility to share and mock the client What is myAmazonSQSClient? Side note, should all the class names be Sqs... or SQS...? I like the latter actually.
          Hide
          Mark Ford added a comment -

          @Christian - I'll create the issue to add my camel-sns component and coordinate with Tracy.

          @Tracy - client=#myAmazonSQSClient is a property on the endpoint URI that allows a developer to configure the component with their own SQS client object. The developer would have configured the spring context or camel registry with a bean mapped to that key. The component checks that property and uses the client object there instead of creating their own. This allows the developer to handle the mock objects or otherwise override the behavior for the API. Obviously the object they provide must implement the same interface or extend the same class as what you're expecting for the Amazon API object.

          Show
          Mark Ford added a comment - @Christian - I'll create the issue to add my camel-sns component and coordinate with Tracy. @Tracy - client=#myAmazonSQSClient is a property on the endpoint URI that allows a developer to configure the component with their own SQS client object. The developer would have configured the spring context or camel registry with a bean mapped to that key. The component checks that property and uses the client object there instead of creating their own. This allows the developer to handle the mock objects or otherwise override the behavior for the API. Obviously the object they provide must implement the same interface or extend the same class as what you're expecting for the Amazon API object.
          Hide
          Tracy Snell added a comment -

          Hah, that's kind of obvious post morning caffeine intake!

          It's possible they've configured the AWS keys in the client also. Should we removed the keys as required parameters? Of course they could just set them to random values to. If they could optionally set the keys then the code would need to support that but should be very easy.

          Show
          Tracy Snell added a comment - Hah, that's kind of obvious post morning caffeine intake! It's possible they've configured the AWS keys in the client also. Should we removed the keys as required parameters? Of course they could just set them to random values to. If they could optionally set the keys then the code would need to support that but should be very easy.
          Hide
          Tracy Snell added a comment -

          Creating a new Jira for the SNS code.

          Show
          Tracy Snell added a comment - Creating a new Jira for the SNS code.
          Hide
          Tracy Snell added a comment -

          Christian, have you made any other changes?

          I'm merging in Mark's code with mine and I'm just going to use his repo at Google code until I have a new patch with both SQS and SNS in it.

          Show
          Tracy Snell added a comment - Christian, have you made any other changes? I'm merging in Mark's code with mine and I'm just going to use his repo at Google code until I have a new patch with both SQS and SNS in it.
          Hide
          Christian Müller added a comment -

          Attached the patch for SMX4-723

          Show
          Christian Müller added a comment - Attached the patch for SMX4-723
          Hide
          Christian Müller added a comment -

          Tracy, I would prefer a new patch for it because I made some other changes:

          • refactored the functionality of SqsQueue into SqsEndpoint, SqsProducer and SqsConsumer because I think we don't need this additional abstraction
          • SqsConsumer: delete the message after successful processing in Camel if so configured (not before)
          • SqsConfiguration: rename delete in deleteAfterRead
          • SqsConsumer + SqsProducer: remove the endpoint instance variable because the super class hold this reference
          • ...
          Show
          Christian Müller added a comment - Tracy, I would prefer a new patch for it because I made some other changes: refactored the functionality of SqsQueue into SqsEndpoint, SqsProducer and SqsConsumer because I think we don't need this additional abstraction SqsConsumer: delete the message after successful processing in Camel if so configured (not before) SqsConfiguration: rename delete in deleteAfterRead SqsConsumer + SqsProducer: remove the endpoint instance variable because the super class hold this reference ...
          Hide
          Tracy Snell added a comment -

          That's fine.

          SqsQueue was just to make testing a bit easier. AWS has screwed up my account and I can't get access to SNS right now anyway.

          Show
          Tracy Snell added a comment - That's fine. SqsQueue was just to make testing a bit easier. AWS has screwed up my account and I can't get access to SNS right now anyway.
          Hide
          Christian Müller added a comment -

          @Tracy, don't worry about this. At present I have enough to do..

          Show
          Christian Müller added a comment - @Tracy, don't worry about this. At present I have enough to do..
          Hide
          Christian Müller added a comment -

          @Claus: Do we add new components (like this one) and provide the feature definition later (maybe with a separate issue)?

          I ask because I think I could finish this work in the next days, but I think the ServiceMix release of OSGI-fied version from aws-java-sdk (what we need for the feature definition) will take some more time. I think it will not be available when we release Camel 2.6.
          I would prefer to add this component to Camel 2.6 and provide the feature definition for 2.7 (with a separate issue). Then our users can use this new component in Camel 2.6 in non OSGI environments and provide temporarily its own feature definition in OSGI environments when the OSGI bundle of aws-java-sdk is released from the ServiceMix guys (or they use the SNAPSHOT version).

          Show
          Christian Müller added a comment - @Claus: Do we add new components (like this one) and provide the feature definition later (maybe with a separate issue)? I ask because I think I could finish this work in the next days, but I think the ServiceMix release of OSGI-fied version from aws-java-sdk (what we need for the feature definition) will take some more time. I think it will not be available when we release Camel 2.6. I would prefer to add this component to Camel 2.6 and provide the feature definition for 2.7 (with a separate issue). Then our users can use this new component in Camel 2.6 in non OSGI environments and provide temporarily its own feature definition in OSGI environments when the OSGI bundle of aws-java-sdk is released from the ServiceMix guys (or they use the SNAPSHOT version).
          Hide
          Christian Müller added a comment -

          Hello Tracy,
          I attached the patch which I will commit after Claus give me his ok. Feel free to test it. Every feedback is welcome.

          Show
          Christian Müller added a comment - Hello Tracy, I attached the patch which I will commit after Claus give me his ok. Feel free to test it. Every feedback is welcome.
          Hide
          Claus Ibsen added a comment - - edited

          Code review
          ==========

          1)
          Prefer to throw IllegalArgumentException instead of CamelException for invalid configuration.
          eg: throw new CamelException("Queue name not specified.");

          2)
          There is a <a/> in the javadoc of SqsConfiguration

          3)
          Why do you use Integer and not int for the properties in SqsConfiguration? You use boolean. Maybe use Boolean so they are all non primitive types, or use int, so they are all primitive.

          4)
          The consumer is scheduled poll based - and it polls X number of messages. Maybe it can be a BatchConsumer as well?
          See the camel-mail component for an example.

          5)
          The logic to delete message after read in the consumer, should preferred to be in a on completion. This ensures it works well with the asynchronous routing engine. See MailConsumer for an example.

          6)
          Prefer to use handleException(e); instead of logging exceptions which was thrown. This allows to re-use logic to handle this. See MailConsumer for example.

          7)
          In the producer you don't propagate headers if its a InOut MEP, in the getMessageForResponse method. You most likely want to copy the headers from the IN message to OUT to preserve those. If you dont do this, then end users will suffer from the "where the f*** is my headers." issue.

          Show
          Claus Ibsen added a comment - - edited Code review ========== 1) Prefer to throw IllegalArgumentException instead of CamelException for invalid configuration. eg: throw new CamelException("Queue name not specified."); 2) There is a <a/> in the javadoc of SqsConfiguration 3) Why do you use Integer and not int for the properties in SqsConfiguration? You use boolean. Maybe use Boolean so they are all non primitive types, or use int, so they are all primitive. 4) The consumer is scheduled poll based - and it polls X number of messages. Maybe it can be a BatchConsumer as well? See the camel-mail component for an example. 5) The logic to delete message after read in the consumer, should preferred to be in a on completion. This ensures it works well with the asynchronous routing engine. See MailConsumer for an example. 6) Prefer to use handleException(e); instead of logging exceptions which was thrown. This allows to re-use logic to handle this. See MailConsumer for example. 7) In the producer you don't propagate headers if its a InOut MEP, in the getMessageForResponse method. You most likely want to copy the headers from the IN message to OUT to preserve those. If you dont do this, then end users will suffer from the "where the f*** is my headers." issue.
          Hide
          Christian Müller added a comment -

          Claus, thanks for the review. I will work on it today evening.
          After that, I will commit this new component so it is part of the 2.6 release. Jean-Baptiste also resolved SMX4-723. This means I can provide the features definition with a dependency to a snapshot version of the aws-java-sdk OSGI bundle from ServiceMix. Maybe we can force a release version of the aws-java-sdk OSGI bundle before Camel 2.6 will be released.

          Show
          Christian Müller added a comment - Claus, thanks for the review. I will work on it today evening. After that, I will commit this new component so it is part of the 2.6 release. Jean-Baptiste also resolved SMX4-723 . This means I can provide the features definition with a dependency to a snapshot version of the aws-java-sdk OSGI bundle from ServiceMix. Maybe we can force a release version of the aws-java-sdk OSGI bundle before Camel 2.6 will be released.
          Hide
          Claus Ibsen added a comment -

          Christian it only makes sense to implement BatchConsumer if it makes sense from AWS point of view. For example it would read a lot of messages, and with the batch consumer you may want to limit that to eg X number of messages per time.

          For example a mailbox may have 10000 mails, and you may want to limit it to 100 mails / poll. Likewise for file consumer etc.
          So if AWS has the same "issue". then implement the BatchConsumer. If not then its fine as is.

          Show
          Claus Ibsen added a comment - Christian it only makes sense to implement BatchConsumer if it makes sense from AWS point of view. For example it would read a lot of messages, and with the batch consumer you may want to limit that to eg X number of messages per time. For example a mailbox may have 10000 mails, and you may want to limit it to 100 mails / poll. Likewise for file consumer etc. So if AWS has the same "issue". then implement the BatchConsumer. If not then its fine as is.
          Hide
          Claus Ibsen added a comment -

          And btw yes it makes sense to push it for 2.6 even if we dont have OSGi bundles and features file. There are a lot of end users not using OSGi and it helps get it out and used by end users, which may find bugs and provide ideas for improvements.

          Show
          Claus Ibsen added a comment - And btw yes it makes sense to push it for 2.6 even if we dont have OSGi bundles and features file. There are a lot of end users not using OSGi and it helps get it out and used by end users, which may find bugs and provide ideas for improvements.
          Hide
          Hadrian Zbarcea added a comment -

          Yes, it should implement BatchConsumer. I have some concerns about testing this component though.
          It is not easy to test with good feature coverage, it depends on aws and could be time consuming. Attention to such details is highly appreciated.

          Show
          Hadrian Zbarcea added a comment - Yes, it should implement BatchConsumer. I have some concerns about testing this component though. It is not easy to test with good feature coverage, it depends on aws and could be time consuming. Attention to such details is highly appreciated.
          Hide
          Christian Müller added a comment -

          Hadrian, I think we covered this. We build integration tests which can be executed manualy to test the behavior against the real AWS (annotated with @Ignore). And we mocked the Amazon SQS client and uses this mock in our real unit tests to not depend on the real AQS.

          Show
          Christian Müller added a comment - Hadrian, I think we covered this. We build integration tests which can be executed manualy to test the behavior against the real AWS (annotated with @Ignore). And we mocked the Amazon SQS client and uses this mock in our real unit tests to not depend on the real AQS.
          Hide
          Christian Müller added a comment -

          My code review comments:
          1) fixed as you suggested
          2) ok, removed
          3) Because I have to differentiate between unset and default values. I changed the code to use only the wrapper types (Boolean, Integer, ...)
          4) ok, implement the BatchConsumer and ShutdownAware interfaces
          5) ok, implemented as suggested
          6) not needed anymore
          7) ok, fixed

          I will add some more unit tests for the BatchConsumer stuff before I commit this.

          Show
          Christian Müller added a comment - My code review comments: 1) fixed as you suggested 2) ok, removed 3) Because I have to differentiate between unset and default values. I changed the code to use only the wrapper types (Boolean, Integer, ...) 4) ok, implement the BatchConsumer and ShutdownAware interfaces 5) ok, implemented as suggested 6) not needed anymore 7) ok, fixed I will add some more unit tests for the BatchConsumer stuff before I commit this.
          Hide
          Christian Müller added a comment -

          aws-java-sdk use jackson-core-asl 1.6.4

          Show
          Christian Müller added a comment - aws-java-sdk use jackson-core-asl 1.6.4
          Hide
          Christian Müller added a comment -

          Added with revision 1055335

          Thank you Tracy for the contribution!

          Show
          Christian Müller added a comment - Added with revision 1055335 Thank you Tracy for the contribution!
          Hide
          Claus Ibsen added a comment -

          Christian, are you sure you must do during a for loop?

                iterator.remove();
          

          That would mean you would skip every 2nd because you remove the next during the looping.
          If its on purpose, then please add a code comment why.

          I think I spotted this in a mock class in this commit.

          Show
          Claus Ibsen added a comment - Christian, are you sure you must do during a for loop? iterator.remove(); That would mean you would skip every 2nd because you remove the next during the looping. If its on purpose, then please add a code comment why. I think I spotted this in a mock class in this commit.
          Hide
          Christian Müller added a comment -

          Claus I think my code works fine. It removes one element for each iteration (not every 2nd). The Java doc says:

              /**
               * 
               * Removes from the underlying collection the last element returned by the
               * iterator (optional operation).  This method can be called only once per
               * call to <tt>next</tt>.  The behavior of an iterator is unspecified if
               * the underlying collection is modified while the iteration is in
               * progress in any way other than by calling this method.
               *
               * @exception UnsupportedOperationException if the <tt>remove</tt>
               *		  operation is not supported by this Iterator.
               
               * @exception IllegalStateException if the <tt>next</tt> method has not
               *		  yet been called, or the <tt>remove</tt> method has already
               *		  been called after the last call to the <tt>next</tt>
               *		  method.
               */
              void remove();
          
          Show
          Christian Müller added a comment - Claus I think my code works fine. It removes one element for each iteration (not every 2nd). The Java doc says: /** * * Removes from the underlying collection the last element returned by the * iterator (optional operation). This method can be called only once per * call to <tt>next</tt>. The behavior of an iterator is unspecified if * the underlying collection is modified while the iteration is in * progress in any way other than by calling this method. * * @exception UnsupportedOperationException if the <tt>remove</tt> * operation is not supported by this Iterator. * @exception IllegalStateException if the <tt>next</tt> method has not * yet been called, or the <tt>remove</tt> method has already * been called after the last call to the <tt>next</tt> * method. */ void remove();

            People

            • Assignee:
              Christian Müller
              Reporter:
              Tracy Snell
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development