calrissian / mango

Common utilities for rapid application development
Apache License 2.0
17 stars 7 forks source link

EventIndex needs a single argument constructor #87

Closed cjnolet closed 10 years ago

eawagner commented 10 years ago

I am guessing this single argument should be timestamp?

cjnolet commented 10 years ago

No. The single argument is for when a user knows an id but doesn't know the timestamp.

On Thu, Jun 19, 2014 at 12:06 PM, eawagner notifications@github.com wrote:

I am guessing this single argument should be timestamp?

— Reply to this email directly or view it on GitHub https://github.com/calrissian/mango/issues/87#issuecomment-46580830.

eawagner commented 10 years ago

Just wondering how this makes sense on an Index class. Maybe I am missing something.

If you don't need a timestamp on an EventIndex then why is it there at all? Are you just trying to use it as a partial index in a special case?

cjnolet commented 10 years ago

It's not that i'm "trying" to use it as a partial index, it's that when I created the class, that was its index. Keep in mind, the index was kind of an event store specific thing but we moved it into the main model so that we could reuse it in other cases. Is there a problem with only representing an id without a timestamp?

On Thu, Jun 19, 2014 at 5:03 PM, eawagner notifications@github.com wrote:

Just wondering how this makes sense on an Index class. Maybe I am missing something.

If you don't need a timestamp on an EventIndex then why is it there at all? Are you just trying to use it as a partial index in a special case?

— Reply to this email directly or view it on GitHub https://github.com/calrissian/mango/issues/87#issuecomment-46616294.

cjnolet commented 10 years ago

The funny thing is, the logic is already in the EventGlobalIndex to check if getTimestamp() is null so that it knows to grab the timestamp of the event by its id... but this would never have occurred because the constructor wasn't on the class in order for the timstamp to ever be null. (the timestamp argument in the constructor is a primative long instead of an Object long).

On Thu, Jun 19, 2014 at 5:07 PM, Corey Nolet cjnolet@gmail.com wrote:

It's not that i'm "trying" to use it as a partial index, it's that when I created the class, that was its index. Keep in mind, the index was kind of an event store specific thing but we moved it into the main model so that we could reuse it in other cases. Is there a problem with only representing an id without a timestamp?

On Thu, Jun 19, 2014 at 5:03 PM, eawagner notifications@github.com wrote:

Just wondering how this makes sense on an Index class. Maybe I am missing something.

If you don't need a timestamp on an EventIndex then why is it there at all? Are you just trying to use it as a partial index in a special case?

— Reply to this email directly or view it on GitHub https://github.com/calrissian/mango/issues/87#issuecomment-46616294.

eawagner commented 10 years ago

There is no problem with representing an index an index as just an id. I just want to make sure that this is the right solution to the problem. For instance maybe just using an object that implements the identifiable interface would suffice.

Even if you do need to have an EntityIndex object, should this common object provide a default value for timestamp (as it is a primitive not an object)? Or does it make sense to have the piece of code trying to represent a partial index supply the constructor with a dummy timestamp value that it will know to ignore if it encounters it?

Even if we do it in the constructor, what dummy value should we use since theoretically all longs represent a valid date? Maybe use Long.MIN_VALUE to represent a timestamp that should be ignored?

Again, I just want to make sure it makes sense to do it on the data object, and maybe this should be done closer to the EventGlobalIndex implementation to handle the special logic of using a dummy timestamp value in an index.

cjnolet commented 10 years ago

timestamp should not be primitive. That's what needs to change. It should be null.

eawagner commented 10 years ago

I missed that the temporal interface returned an actual Long instead of a primitive.

I am fine with trying to take advantage of using null, but for consistency sake we should also change the Event class to have the same behavior to be consistent with all Temporal implementations.

I think right now it defaults to System.currentMillis(), but in reality if this value should be nullable to be consistent and it should be up to the consumer of the timestamp information as to what to do with a null value.

cjnolet commented 10 years ago

Umm. I'm not sure if I agree with that. An event should never not have a timestamp as it's something that defines it. The eventIndex on the other hand it's possible to not have a timestamp because the person referencing the event may not know it...

eawagner commented 10 years ago

So I think the agreed upon path was to make Temporal use a primitive long and to not have EntityIndex implement Temporal.

Then for EntityIndex we return a nullable long for the timestamp so that it can be used to represent partial indexes.