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

getLastEventOfEachAggregate, onBeforeSet middleware #90

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

robinfehr
Copy link
Contributor

  • set data to rev guard store via onBeforeSet middleware
  • new cocatenatedId format with delimiters to better parse the value
  • get data from rev guard store via exposed getLastEventsOfEachAggregate
    fn

- set data to rev guard store via onBeforeSet middleware
- new cocatenatedId format with delimiters to better parse the value
- get data from rev guard store via exposed getLastEventsOfEachAggregate
  fn
@adrai
Copy link
Contributor

adrai commented Jun 12, 2020

  1. Maybe clarify with some comments, what is the difference between: prefix (https://github.com/adrai/node-cqrs-eventdenormalizer/pull/90/files?file-filters%5B%5D=.js&file-filters%5B%5D=.md#diff-04c6e90faac2675aa89e2176d2eec7d8R100) and prefix (https://github.com/adrai/node-cqrs-eventdenormalizer/pull/90/files#diff-0745fa2041110cff3082322255437920R499)

  2. can there be a possibility to optionally omit the prefix and keep the old api? https://github.com/adrai/node-cqrs-eventdenormalizer/pull/90/files#diff-ab712d0ee8b0b73d36fe730eeefa8bebR452
    btw. Maybe changing the signature is not needed at all, because the prefix is in the options?

  3. add some info to the readme or changelog regarding v2 breaking changes (also for those directly using the store functionality...)

  4. docs for getLastEventOfEachAggregate in the readme

@adrai adrai requested a review from nanov June 12, 2020 06:17
},

getLastEventOfEachAggregate: function (callback = (err, aggregateHandleFns) => {}) {
this.revisionGuardStore.getValueOfEachKey(this.options.revisionGuard.prefix, callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

idk, getValueOfEachKey sounds very specific to the redis implementation...
What about mongodb, etc...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how would you like getValueOfEachId?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nanov Any better idea?

Copy link
Contributor Author

@robinfehr robinfehr Jun 21, 2020

Choose a reason for hiding this comment

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

I will now add the getValueOfEachId version and update the PR :)

@nanov
Copy link
Contributor

nanov commented Jun 13, 2020

can you explain the purpose of the PR a bit?
Sorry but, by glance, it seems as you are trying to solve a problem in the wrong domain :/

@robinfehr
Copy link
Contributor Author

robinfehr commented Jun 14, 2020

can you explain the purpose of the PR a bit?
Sorry but, by glance, it seems as you are trying to solve a problem in the wrong domain :/

we use the even'ts occuredAt date to fetch events that we missed. Our implementation of the queue / bus to our denormalizers are not persistent and therefore it can happen that we miss some events. We then fetch them using a replayer service - common pattern - to which we send a request to retrieve the missed events beginning a certain from date to the last saved event in the store. We have to find the oldest / smallest occuredAt date for all our aggregates in the denormalizer. To do so, having only the revision wihtin the revision guard per aggregate, we'd have to make a query to the eventstore (in our case via the replayer service) for each aggregate and it's revision to retrieve the from date. Putting that traffic onto the denormalizers db is better since it won't affect the evenstore and therefore the rest of the ecosystem. To achieve that the middleware got added, so that we can save the occuredAt or any other data to the denormalizers db and off-load the traffic from the eventstore. The last_seen_event can't be used since the handle fn is per aggregate and therefore an other aggregate can be behind the last_seen_event's time or any other meta data that would be stored there.

@robinfehr
Copy link
Contributor Author

  1. Maybe clarify with some comments, what is the difference between: prefix (https://github.com/adrai/node-cqrs-eventdenormalizer/pull/90/files?file-filters%5B%5D=.js&file-filters%5B%5D=.md#diff-04c6e90faac2675aa89e2176d2eec7d8R100) and prefix (https://github.com/adrai/node-cqrs-eventdenormalizer/pull/90/files#diff-0745fa2041110cff3082322255437920R499)
  2. can there be a possibility to optionally omit the prefix and keep the old api? https://github.com/adrai/node-cqrs-eventdenormalizer/pull/90/files#diff-ab712d0ee8b0b73d36fe730eeefa8bebR452
    btw. Maybe changing the signature is not needed at all, because the prefix is in the options?
  3. add some info to the readme or changelog regarding v2 breaking changes (also for those directly using the store functionality...)
  4. docs for getLastEventOfEachAggregate in the readme
  1. it is the same. the prefix can still be specified within the revisiongard's options. api-wise nothing changed. just the default prefix is now different.

  2. the old api is still intact. the prefix can be omitted. the signature of the stores got changed so that they're agnostic towards the denormalizer's options and threfore can be unit tested properly.

  3. will do :)

  4. will do :)

@adrai
Copy link
Contributor

adrai commented Jun 18, 2020

  1. I mean the store signature...
    Why not keep the old signature and use this.options.prefix within the function?

callback(null, new ObjectID().toString());
},

get: function (id, callback) {
get: function (prefix, id, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i.e. Instead of this...
keep the old signature

get: function (id, callback) {
  // and within the function
  this.options.prefix
  // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it made the untit testing easier because we don't have to connect again to test the prefix itself more isolated.

also, it makes it more agnostic towards the denormalizer functionality. e.g. when we want to add a sharding feature, one denormalizer might need to handle multiple prefixes. also,

I thought it is more transparent/ guessable when e.g. using the "set" function to see what kind of key will be generated.

these were the thoughts when implementing it.

I don't mind changing the signature back - for our current use case, both works. Just let me know if I should.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nanov What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nanov IMO having the same interface as before and working with this.options.prefix sounds more suitable for me...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any decision so far? Else i'll just change it back to the old api and hope to get it merged soon then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants