Skip to content

INTEXT-36 - Voldemort Adapter#20

Closed
lukasz-antoniak wants to merge 5 commits into
spring-projects:masterfrom
lukasz-antoniak:INTEXT-36
Closed

INTEXT-36 - Voldemort Adapter#20
lukasz-antoniak wants to merge 5 commits into
spring-projects:masterfrom
lukasz-antoniak:INTEXT-36

Conversation

@lukasz-antoniak

Copy link
Copy Markdown
Contributor

Patch for INTEXT-36 JIRA ticket.
Link: https://jira.springsource.org/browse/INTEXT-36

Regards,
Lukasz Antoniak

@amolnayak311

Copy link
Copy Markdown
Contributor
  • Remove all jars from the project, these need to be a part of the dependencies and should not be placed in the repository.
  • Change the year in all the class to 2013 for the Licence in the header.

@lukasz-antoniak

Copy link
Copy Markdown
Contributor Author

@amolnayak311: No, you cannot exclude those jar libraries. Voldemort is not build with Maven, and is not being uploaded to Maven Central Repository. Those libraries are required for integration tests to run against local Voldemort database. Otherwise, you would have to download them manually, and simple gradle test would not work after clean checkout.

Will change the year to 2013 shortly.

@amolnayak311

Copy link
Copy Markdown
Contributor

The commons, jackson, thift, jetty and I guess all others can come from maven repo. For voldemort jars, dont worry they can be put in Spring source's mvn repository. So eventually we will have to get the jars from the repository and none with the project's source code.

@lukasz-antoniak

Copy link
Copy Markdown
Contributor Author

Got your point and it makes sense. What do you think about referencing Voldemort 0.96 with something like compile "voldemort:voldemort:$voldemortVersion", plus installing the following POM descriptor: https://gist.github.com/4546021 (all dependencies included)? I have tested it locally and everything works fine.

If you agree, could you please install Voldemort 0.96 with the given POM in Spring's Maven repository? I will test it afterwards and update build.gradle, remove libraries and .gitignore file.

@lukasz-antoniak

Copy link
Copy Markdown
Contributor Author

Hello Spring Team, any recent updates?

@ghillert

Copy link
Copy Markdown
Contributor

Hi Lukasz,

I have just added Voldemort to the SpringSource repository - voldemort:voldemort:0.96.

Cheers,

Gunnar

@lukasz-antoniak

Copy link
Copy Markdown
Contributor Author

Thanks Gunnar, looks good from my working directory :).

@lukasz-antoniak

Copy link
Copy Markdown
Contributor Author

Hello Spring Team,

In my opinion message store cannot be easily implemented, because Voldemort does not support transactions and row locking - AP system in terms of CAP theorem. During functional testing, I faced issues with concurrent modifications of single message group by two separate message store clients. BTW, any updates regarding code review and documentation?

Regards,
Lukasz

@artembilan

Copy link
Copy Markdown
Member

Hello, Lukasz.
As you may know MongoDB doesn't support TX as well, however we have its MessageStore implementation.
From other side it is anti-pattern to access to the same MessageGroup from different applications...
To achieve exclusive access to the MessageGroup we use LockRegistry: take a look into AbstractCorrelatingMessageHandler.

Cheers

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for introducing this new interface and not using _o.s.i.s.c.MessageConverter_ instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason standing behind this decision was type safety and clarity. I didn't want user to realize in runtime (or while reading documentation) that the object he receives in MessageConverter#toMessage(Object) method is actually KeyValue<K, Versioned<V>> wrapper, and that I expect him to return KeyValue<K, V> in MessageConverter#fromMessage(Message<P>). If you can think of any better solution or using MessageConverter is preferred, I'll be happy to alter this part of code. I can see that spring-integration-mongodb module also uses MongoDB specific converter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you say makes sense and yes, Mongo adapters do use a custom converter, but that comes from spring data mongo and is used to instantiate the MongoTemplate if one is not provided.
I am comparing this more to the Redis's _o.s.i.r.oRedisStoreWritingMessageHandler_ as that would be a better comparison.
Merging in a way you have implemented shouldn't be a issue in my opinion as we can anytime change things in future to accommodate the changes.But do take a look at the way Redis's adapters are implemented and let us know your views.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VoldemortConverter allows user to store chosen part of message payload. After quick skip through the implementation of RedisStoreWritingMessageHandler, I guess that it can persist only complete payload objects. Currently I think that payload transformation could be processed by @Transformer outside of Voldemort adapter, so let's omit this issue. More important design decision is the way of determining object's key. This is achieved programmatically in VoldemortConverter, while Redis adapter looks up concrete message header (RedisHeaders.KEY) or evaluates key-expression.

What would be the preferred way of passing/calculating object's key in your opinion? In case it's Redis' approach, I will change Voldemort adapter to work similarly. IMO +1 for Redis.

@amolnayak311

Copy link
Copy Markdown
Contributor

@lukasz-antoniak Apologies for not getting this merged quickly. I want to go through this and review it but I am tied up with some other stuff and unable to do this. Please do not feel that your contribution is not valued :)
It might take some time but it should be merged. Somebody else might review too as per their convenience.
Thanks again for the contribution.

@lukasz-antoniak

Copy link
Copy Markdown
Contributor Author

I have refactored Voldemort adapter to calculate entry key analogically to Redis module. IMO design looks better now, as there is no extra interface and all you need to code is something like store-key-expression="payload.id". Thank you @amolnayak311 for this suggestion. Any progress with reviewing the rest of the code?

@lukasz-antoniak

Copy link
Copy Markdown
Contributor Author

Hello Spring Team, any recent updates?

ghillert added a commit to ghillert/spring-integration-extensions that referenced this pull request Apr 2, 2013
* lukasz-antoniak-INTEXT-36:
  INTEXT-61 - Add Voldemort Sample
  INTEXT-36 - Cleanup
  INTEXT-36 - Add Voldemort Module
@ghillert

ghillert commented Apr 2, 2013

Copy link
Copy Markdown
Contributor

Did some code cleanup:

  • Convert spaces to tabs
  • Upgrade to Gradle 1.5
  • Change Group Id to 'org.springframework.integration'
  • Add Sonar Runner plugin
  • Reduce Sonar warnings
  • Ensure ASL license header is present in all source files
  • Fix XSD documentation not showing up in STS

Also, I added a (very simple) Voldemort sample to the repo:

Furthermore, I have setup the CI infrastructure:

The Sonar Analytics dashboard is here:

The snapshot artifacts are automatically deployed to:

THANK YOU for the contribution! Looking forward to new PRs!

@ghillert ghillert closed this Apr 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants