sky-uk / cqlmigrate

Cassandra schema migration library

BSD 3-Clause "New" or "Revised" License

47 stars 29 forks source link

Locking #1

Closed adamdougal closed 8 years ago

chbatey commented 8 years ago

Taking a look at this today. Can't merge to master until the testing that we described is done.

chbatey commented 8 years ago

Comments inline.

One general comment is unless you need Java 7 can we make this Java 8 and remove joda from the public API? Teams wishing to use this can upgrade to Java 8 as it trivial for services. We can of course continue to support Java 7 for external libraries like umv client.

jsravn commented 8 years ago

Can we get rid of the dependency on common-lang3? We already have a guava dependency which should be sufficient for any utility stuff we need. I'd like to avoid adding too many dependencies here (even guava is arguably a lot - and we should consider jarjaring it) - it makes the library less usable by other people.

jsravn commented 8 years ago

This resolves #10 and resolves #8.

cirrus-core commented 8 years ago

A general comment for any instance where we log an exception at source and re-throw another exception, to not put the original exception in the cause. This will cause exceptions to be double logged.

If we decide not to log at source then putting it as a cause is a good idea.

jsravn commented 8 years ago

Apart from a few minor things it's all looking very good. Close to merging I think, good work guys!

adamdougal commented 8 years ago

We just pushed the last of the agreed changes to the branch. @balooo, @chbatey could you please review them?

chbatey commented 8 years ago

Lock.java

LockingMechanism strictly not necessary as there is only one impl. Having any Cassandra specifics in the constructor would have sufficed as then the interface is clean. We can leave it in tho.
I like your RetryTask. Similar to something in Guava but we've removed that dependency which is a good thing.

Testing

Does this pass locally for you? shouldObtainLockToInsertRecord

balooo commented 8 years ago

We need to make sure that the way we release this doesn't clash with our internal cqlmigrate tool (naming/packaging)

oliverlockwood commented 8 years ago

@balooo the current dependency as pulled in by umv-service is com.bskyb.internettv.cirrus:cqlmigrate:0.7

It's perhaps worth specifying the groupId explicitly in the build.gradle for this model? At the moment I believe that https://github.com/sky-uk/cqlmigrate/blob/master/settings.gradle#L1 will set the artifact name to cqlmigrate, but I'm not sure what'll be defining the groupId.