onyx-datomic icon indicating copy to clipboard operation
onyx-datomic copied to clipboard

read-log doesn't end when using tx ids

Open jeremyheiler opened this issue 7 years ago • 9 comments

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.

jeremyheiler avatar Mar 24 '17 15:03 jeremyheiler

cc @lbradstreet

MichaelDrogalis avatar Mar 24 '17 15:03 MichaelDrogalis

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 [email protected] 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 avatar Mar 25 '17 00:03 lbradstreet

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

lbradstreet avatar Apr 03 '17 23:04 lbradstreet

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.

jeremyheiler avatar Apr 04 '17 17:04 jeremyheiler

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

On 4 April 2017 at 10:40, Jeremy Heiler [email protected] 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 .

lbradstreet avatar Apr 04 '17 17:04 lbradstreet

No worries! :-)

jeremyheiler avatar Apr 04 '17 18:04 jeremyheiler

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?

jeremyheiler avatar Apr 04 '17 21:04 jeremyheiler

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.

lbradstreet avatar Apr 05 '17 06:04 lbradstreet

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..

jeremyheiler avatar Apr 05 '17 12:04 jeremyheiler