search

scalar-labs / btm

JTA Transaction Manager
Apache License 2.0
426 stars 151 forks source link

Add unique ID to compareTo calculation on Task object (closes #67) #68

Closed pgbsl closed 8 years ago

pgbsl commented 8 years ago

Fix proposal for issue #67

I have used UUID implementation as discriminator. Any thoughts on a better approach?

lorban commented 8 years ago

You could replace the UUID with a simple int generated from a AtomicInteger. Other than that, this change looks good - thanks for it!

pgbsl commented 8 years ago

100% agree. I was overthinking the unique ID side of things. Have commited with an int uniqueId, sourced from an AtomicInteger

pgbsl commented 8 years ago

Ignore that last commit. Travis build is using JDK 1.6, and the Integer.compare(int, int) method I used is JDK 1.7 feature.

I will use 1.6 code instead

lorban commented 8 years ago

Could you please rebase your branch and squash the four commits together since you're not using a UUID anymore to solve the bug?

Thanks!

pgbsl commented 8 years ago

Squashed and rebased as requested (thanks for the travis JDK 6 fix).

Are you happy with the level of testing (i.e. the new test added to TaskSchedulerTest.java). Happy to add in a new TaskTest class (this would probably re-use the SimpleTask defined inside TaskSchedulerTest, so I would need to promote that to a top-level public class). This could then fully test the various paths that compare to will go through

lorban commented 8 years ago

I've added a comment in your test but other than that, it looks like this would be enough testing. Do you have other tests in mind you'd want to add?

pgbsl commented 8 years ago

Yeah - sorry about that test case. Not very robust. Now using a shared timestamp. I don't think that any further test cases are needed. We could add explicit boundary tests for the compareTo method (i.e. differentTimestamps, equalTimestamps but different uniqueId, comparison with self. But i don't think this is strictly necessary