spring-projects / spring-integration-extensions

The Spring Integration Extensions project provides extension components for Spring Integration
http://www.springintegration.org/
279 stars 265 forks source link

Intext 50 - SI Cassandra outbound adapter - still in the initial phases #143

Closed sobychacko closed 9 years ago

artembilan commented 9 years ago

Hello @sobychacko !

First of all thank you very much for such a hard work! Here you are the polishing from my perspective: https://github.com/artembilan/spring-integration-extensions/commit/e25951d19c289b1dc943ef18c842233d53b3ca5c.

What I see you haven't finished CassandraOutboundGateway. But it isn't an issue and we always can address all others as separate JIRAs/PRs.

Let me share with you some concerns which I feel:

  1. Having such a generic Gateway, I'd say that there is no reason to have that CassandraStoringMessageHandler, because with requiresReply = false on the AbstractReplyProducingMessageHandler we always can ignore that Cancellable result.
  2. I think it would be better to add some table option (or something other) and not rely on the @Table from the provided entity. Because it is the fact that SI doesn't like to get deal with Domains. And it would be great to ingest some generic objects like Map. Of course we can achieve that with upstream <transformer>, but just a thought if you can address it somehow...
  3. The cqlIngest is required when we specify highThroughputIngest. From other side they both are for the same this.cassandraOperations.ingest. So, I don't see reason in that highThroughputIngest, because cqlIngest covers that as well.
  4. Would you mind explaining the reason of that CassandraStoringMessageHandlerDriver? Can't we just drop it, as well as the whole Boot dependency ?
  5. You have an issue with indents in your IDE: tabs not 4 whitespaces
artembilan commented 9 years ago

And one more regarding Embedded Cassandra. I see this in logs:

18:13:26.036 WARN  [COMMIT-LOG-ALLOCATOR][org.apache.cassandra.utils.CLibrary] JNA link failure, one or more native method will be unavailable.
18:13:26.933 WARN  [pool-2-thread-1][org.apache.cassandra.db.SystemKeyspace] No host ID found, created 572c0aa2-0533-4b3d-a10a-d3d062e631f3 (Note: This should happen exactly once per node).
18:13:27.113 WARN  [pool-2-thread-1][org.apache.cassandra.service.StorageService] Generated random token [-4656977501133410852]. Random tokens will result in an unbalanced ring; see http://wiki.apache.org/cassandra/Operations
18:13:27.807 ERROR [NonPeriodicTasks:1][org.apache.cassandra.io.sstable.SSTableDeletingTask] Unable to delete build\embeddedCassandra\data\system\local-7ad54392bcdd35a684174e047860b377\system-local-ka-3-Data.db (it will be removed on server restart; we'll also retry after GC)
18:13:27.808 ERROR [NonPeriodicTasks:1][org.apache.cassandra.io.sstable.SSTableDeletingTask] Unable to delete build\embeddedCassandra\data\system\local-7ad54392bcdd35a684174e047860b377\system-local-ka-4-Data.db (it will be removed on server restart; we'll also retry after GC)
18:13:27.809 ERROR [NonPeriodicTasks:1][org.apache.cassandra.io.sstable.SSTableDeletingTask] Unable to delete build\embeddedCassandra\data\system\local-7ad54392bcdd35a684174e047860b377\system-local-ka-2-Data.db (it will be removed on server restart; we'll also retry after GC)
18:13:29.112 WARN  [SharedPool-Worker-1][org.apache.cassandra.utils.FBUtilities] Trigger directory doesn't exist, please create it and try again.
18:13:29.678 ERROR [NonPeriodicTasks:1][org.apache.cassandra.io.sstable.SSTableDeletingTask] Unable to delete build\embeddedCassandra\data\system\schema_keyspaces-b0f2235744583cdb9631c43e59ce3676\system-schema_keyspaces-ka-1-Data.db (it will be removed on server restart; we'll also retry after GC)

Would you mind to explain how to be or provide some route where to take a look to fix them? Or should we just ignore that and continue living with that noise? Although I think that issue with Unable to delete may impact the test class performance.

sobychacko commented 9 years ago

@artembilan Addressed item #1 in the review. Can you take a look please?

sobychacko commented 9 years ago

@artembilan Addressed comments #3, #4 and #5 as well.

CassandraStoringMessageHandlerDriver was a test driver to test the adapter against a stand alone Cassandra rather than invoking the Embedded Cassandra through integration test. It was just for convenience so that I could verify things quickly rather than waiting to write integration tests. Anyway, I removed it and the boot dependency is gone as well.

artembilan commented 9 years ago

Thank you @sobychacko for having a progress. Would mind explain what was the reason do not integrate my polishing to you branch?

git pull artembilan INTEXT-50
sobychacko commented 9 years ago

oh...I am so sorry - i should have pulled first. Will do that.

artembilan commented 9 years ago

@sobychacko , thank you very much for having a progress! Please, be sure to comment on the PR with simple Pushed when you have pushed the new commit to your branch. Otherwise GH doesn't notify us for changes and we may miss your work in time.

artembilan commented 9 years ago

@sobychacko , I took a look at this one more time and really see that single CassandraMessageHandler<T> extends AbstractReplyProducingMessageHandler with producesReply would be enough. For convenience from XML config we can provide separate <outbound-gateway> and <outbound-channel-adapter>.

All your cases like INSERTING, UPDATING, DELETING are good, I also found that generic CassandraTemplate.execute(Statement s) is good as well. We consider with @garyrussell two options: <statement-expression> to allow to use QueryBuilder from SpEL and statement-strategy - an interface to implement and return Statement. That allows us bypass @Table on domain objects and get deal with XD generic types, like Tuple or JSON.

Since I see you don't have time to take care, I'm going to go ahead with this stuff tomorrow to have a chance to bring to XD release this Friday.

garyrussell commented 9 years ago

Just to be clear, we don't really need namespace support for XD; we can make an 0.5.0.RELEASE (with milestone for XD Friday).

artembilan commented 9 years ago

So, I'm merging it as is and go ahead with our requirements in separate fresh branch.

artembilan commented 9 years ago

Merged as https://github.com/spring-projects/spring-integration-extensions/commit/b9089fb419b126ec3b1d5d76c1db5be77e777b13 @sobychacko , you must change your email in GitHub, as well as in Git Client to schacko@pivotal.io