incubator-nuvem-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dulini Atapattu <dulin...@gmail.com>
Subject Re: GSOC 2012 - Message Queue component for Nuvem
Date Mon, 20 Aug 2012 09:44:39 GMT
Hi Jean and all,

Thank you for your suggestions and comments regarding the patch that are a
lot helpful in making the code much effective to the subject.
I have submitted patches to JIRA (NUVEM-14) that includes,

   - re-designed nuvem api
   - changes to the implementation of AmazonSQS

according to Jean s comments and also,

   - implementation for GAE
   - implementation for ActiveMQ.

Please be kind enough to check for the answers I have mentioned for the
questions by Jean.

On Mon, Jul 16, 2012 at 2:05 PM, Jean-Sebastien Delfino <
jsdelfino@apache.org> wrote:

> On Fri, Jul 13, 2012 at 12:25 AM, Dulini Atapattu <dulini88@gmail.com
> >wrote:
>


> Thanks, that helps!
>
> I've reviewed your patch and committed it under SVN revision r1361932.
>
> Here are a few review comments, suggestions and questions:
>
> - I'd suggest to keep the package names lowercase and perhaps shorten
> 'messageQueueService' a little. How about just 'queue'?
>

I have changed the package name to queue

>
> - Also perhaps shorten 'QueueMessage' to 'Message', as there's nothing that
> really ties your message to a queue, and if you just want to indicate that
> this type of message is for use with queues, your package name already
> indicates that.
>
I have changed 'QueueMessage' to 'QMessage' instead of 'Message', as
AmazonSQS already has a class named 'Message' in
com.amazonaws.services.sqs.model which we use in our AmazonSQS
implementation, thus to avoid confusion of our 'QMessage' class with Amazon
s 'Message' class.

>
> - I'm not sure that you need the setter methods on QueueMessage, as your
> constructor already allows to initialize all its properties. It may be
> simpler to just make it immutable.
>

I have made 'QMessage' immutable, by removing the setter messages from the
class.

>
> - I don't think that the following error handling code:
> } catch(Exception e) {
>     throw new MessageQueueServiceException(e);
> }
> actually helps the caller. Why is wrapping all exceptions in a
> MessageQueueServiceException better than just letting the original
> exceptions go through?
>
> I have removed the MessageQueueServiceException class which only wraps the
'Exception' and made the methods in our api throw the original 'Exception'.


> - It seems that the only interesting property in QueueMessageHandle is the
> message ID returned by the queuing service after you send a message. What
> would you think about changing sendMessage to simply return that message ID
> instead of a QueueMessageHandle wrapper object (and then just get rid of
> that QueueMessageHandle class altogether)?
>
> I also removed the class 'QueueMessageHandle' and made the sendMessage
method in the api only returns the send message ID (String), which is the
useful property returned at sending the message.


> - deleteMessage only needs a message ID (which you currently get from the
> message passed in as a parameter). How about just passing the message ID
> directly to deleteMessage instead of requiring the caller to pass the whole
> message (which forces him to keep it around in memory until he's ready to
> call deleteMessage)?
>

I made the deleteMessage method only requires the messageId (String) to be
passed to the method instead of passing the whole message which reduces
memory usage of storing message objects.

>
> - I'd suggest to add a few real unit tests. I mean tests that mockup the
> system dependencies (on EC2, SQS etc) to exercise your code without
> requiring an actual EC2 instance and SQS queue. That way, others in the
> project can verify that your code does what you intended to do when they
> build, without having to set up an EC2 account etc.
>

 I have currently implemented the mock class for AmazonSQS and also the
related test cases and please note that the test cases we have implemented
are capable to run using the mock class.

>
> - Did you manage to implement the same interfaces on GAE? When I initially
> looked at your interfaces I thought that they looked very close to the SQS
> interfaces and was wondering how you'd implement them on GAE... Did you run
> into any issues there?
>

I could also implement the above api to GAE (PullQueue) without issues and
the patch is submitted to JIRA.

Please be kind enough to review the patches and mention comments and
suggestions.

Thanks
Best Regards

>
> Thanks!
>
> - Jean-Sebastien
>



-- 
Dulini I Atapattu
Look around... <http://www.flickr.com/photos/dia1988>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message