Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make default persistent entity offset Long #148

Merged
merged 1 commit into from Aug 26, 2016

Conversation

jroper
Copy link
Member

@jroper jroper commented Aug 23, 2016

The current PersistentEntityRegistry API is specific to Cassandra, since it uses UUID for offsets, where Akka persistence only requires the use of Long.

This change moves the UUID version of the method to a new CassandraPeristentEntityRegistry trait, deprecates the existing method, and creates a new version of the method that uses Long offsets.

In order to keep the change binary compatible, the new method switches the order of parameters around. That's not entirely ideal, since it probably makes more sence to have the startFrom parameter second. However, I couldn't think of a better way, I think eventStream is the best name. I'm open to suggestions if someone can think of a better name or strategy to maintain binary compatibility.

@@ -7,6 +7,7 @@ import java.util.Optional
import java.util.UUID
Copy link
Contributor

Choose a reason for hiding this comment

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

change package to com.lightbend.lagom.internal.persistence.cassandra, where other Cassandra specific classes are located

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@jroper jroper force-pushed the event-stream-uuid branch 2 times, most recently from e888d5c to 278dd0f Compare August 23, 2016 06:30
@@ -38,6 +38,16 @@ trait PersistentEntityRegistry {
* all persistent events of all `Order` entities.
*/
def eventStream[Event <: AggregateEvent[Event]](
Copy link
Contributor

@patriknw patriknw Aug 23, 2016

Choose a reason for hiding this comment

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

@jroper
Copy link
Member Author

jroper commented Aug 25, 2016

@patriknw Using UUID for everything, and converting them to and from longs, introduces a problem in that the same event will have multiple different UUIDs every time it's loaded, since when you convert a long to a UUID, you have to fill in the rest of the bits, which the datastax library fills in with node specific information and a local sequence number, in accordance with the UUID spec. The other problem with it is that it's an incorrect use of UUID - the long from other persistence journals is typically a sequence number, not a timestamp. UUIDs have several different modes which place significance on their components, and applications often use this, so if an application sees that it's a timestamp, when it's actually a sequence number, it's going to end up extracting a timestamp that will be completely wrong.

Polymorphic offset types offer a convenient API, the biggest drawback I can see is that our offset tables will need two columns to store it, a uuid column and a long column, and will have to decide which it is based on which one is not null.

@jroper
Copy link
Member Author

jroper commented Aug 25, 2016

Actually I'm wrong about the multiple different UUIDs, the UUIDs.startOf method will return the same UUID for the same timestamp. But as the library says, these are "fake" UUIDs, since they are not unique, which is just as bad, if not worse, than the other problem, the whole point of UUIDs is you can always assume they are unique, not just for a given entity type, but across all entities on every system everywhere. Passing out non unique UUIDs is a big problem.

@jroper
Copy link
Member Author

jroper commented Aug 25, 2016

Ok, I've updated this to be polymorphic. I've been very strict, the types must strictly be an offset type, that means the UUID must be a time-based UUID (version 1), since that's the only type of UUID that can be used for an offset, and this is enforced. And the long must be a Sequence. There is no conversion of sequences to time based UUIDs since this doesn't make sense - the two types are not comparable in any way and can't be converted in a way that doesn't violate each others rules.

So, Cassandra will only consume and produce UUID offsets, it will fail if it sees a long based sequence, since it doesn't support sequence numbers for offsets. I think this is the best and safest way to implement things.

@jroper jroper force-pushed the event-stream-uuid branch 2 times, most recently from 040e63f to 5e8a04c Compare August 25, 2016 04:57

@Override
public int hashCode() {
return (int) (value ^ (value >>> 32));
Copy link
Contributor

@patriknw patriknw Aug 25, 2016

Choose a reason for hiding this comment

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

reuse Long.hashCode(value)

@patriknw
Copy link
Contributor

Sorry if I confused you with the conversion thoughts. I agree, and what you have implemented is exactly what I had in mind with the Offset type.

LGTM

@patriknw
Copy link
Contributor

Perhaps we can actually change the api of the persistence query eventsByTag api to use something like this. I have previously not wanted to do that because I only saw Any as the possible type, but with a few predefined Offset types it makes more sense.

@jroper jroper merged commit 402812f into lagom:master Aug 26, 2016
@jroper jroper deleted the event-stream-uuid branch August 26, 2016 02:44
Fabszn pushed a commit to Fabszn/lagom that referenced this pull request Aug 28, 2016
Fabszn pushed a commit to Fabszn/lagom that referenced this pull request Sep 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants