search

onyx-platform / onyx-datomic

Onyx plugin for Datomic
http://www.onyxplatform.org/
Eclipse Public License 1.0
37 stars 14 forks source link

read-log doesn't end when using tx ids #26

Open jeremyheiler opened 7 years ago

jeremyheiler commented 7 years ago

If you use tx ids for start and end with read-log, it never ends since it compares the end-tx with t, which is (effectively) always smaller.

https://github.com/onyx-platform/onyx-datomic/blob/0.10.x/src/onyx/plugin/datomic.clj#L200

This leads me to wonder if tx ids are supported? I assumed so based on the name of the keys :datomic/log-start-tx and :datomic/log-end-tx. And both t and tx ids are supported by datomic.api/tx-range.

I'd be happy to provide a patch either way.

MichaelDrogalis commented 7 years ago

cc @lbradstreet

lbradstreet commented 7 years ago

Hi Jeremy,

Yes, this is a bug. We are trying to be consistent with datomic's tx-range call which is inclusive of the start tx and exclusive of the end tx, but I think it makes far more sense to be inclusive of both the start and the end. This would be simpler to understand and use and is more consistent with the other streaming plugins.

I agree that supporting Tx ids would be a good addition.

If you'd like to supply a PR for both of these we'd be very happy to merge it.

Thanks!

On 24 Mar 2017, at 08:52, Michael Drogalis notifications@github.com wrote:

cc @lbradstreet

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

lbradstreet commented 7 years ago

Hi @jeremyheiler, we're still happy to put this in onyx-datomic if you've made any progress. Thanks!

jeremyheiler commented 7 years ago

I'm working on it now. I had to take a break from this, but now I'm back and it's directly blocking me.

lbradstreet commented 7 years ago

Great. I didn't mean to be pushy! Cheers.

On 4 April 2017 at 10:40, Jeremy Heiler notifications@github.com wrote:

Will do! I had to take a break from this, but now I'm back and it's directly blocking me.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/onyx-platform/onyx-datomic/issues/26#issuecomment-291576389, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPZHU16b_X9Zk2xTXFN6oT3WbGipVCGks5rsoB9gaJpZM4MoZ4e .

jeremyheiler commented 7 years ago

No worries! :-)

jeremyheiler commented 7 years ago

Thinking about it a little more, I'm not sure the best way to handle tx ids. My thought would be to normalize them to t values so comparison operations can be done. But is there a good predicate for tx ids? Perhaps byte length?

lbradstreet commented 7 years ago

Normalizing to t values seems reasonable.

Maybe we could make a breaking change and deprecate: :datomic/log-start-tx and :daotmic/log-end-tx.

and then create two sets of task parameters :datomic/log-start-t, :datomic/log-end-t and :datomic/log-start-txid, :datomic/log-end-txid

Then we could also change the behaviour of log-end-t to not be exclusive on the end t?

Alternatively, the task-map could take a param :datomic/log-tx-type :txid or :datomic/log-tx-type :t which switches between them.

jeremyheiler commented 7 years ago

I don't think I have a preference. In the first solution, someone could accidentally mix :datomic/log-start-t with :datomic/log-end-txid. Is there a place to validate that? Although, it'd be easy to normalize the one txid to a t, so that might be OK. Then again, with the second solution, someone could say :txid and provide t values. Bleh..