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

#140 Persist multiple events in batch #141

Merged

Conversation

fbrns
Copy link
Member

@fbrns fbrns commented Sep 27, 2021

#140 Persist multiple events in batch

Sketch for the Release notes:

  • When you fire multiple similar events at once (in the same transaction), you can now use the plural fire*Events methods to store them all in one go into the database, instead of one-by-one. This increases throughput.
  • If you used EventLogRepository.persist directly (you shouldn't!), it was previously storing the database ID of the new object back into the EventLog object. This is not happening anymore (in exchange for more efficiency for bulk inserts).

@fbrns fbrns requested a review from ePaul as a code owner September 27, 2021 09:49
pom.xml Outdated Show resolved Hide resolved
@ePaul
Copy link
Member

ePaul commented Oct 8, 2021

Please also extend the README in the section Creating events.

Comment on lines 55 to 63
* @param dataTypeToData
* the content of the {@code data_type} field of the Nakadi
* event mapped to some POJOs that can be serialized into JSON (required
* parameter). This is meant to be a representation of the
* current state of the resource. It will be used as content of
* the {@code data} field of the Nakadi event.
*/
@Transactional
void fireCreateEvents(String eventType, Map<String, Collection<Object>> dataTypeToData);
Copy link
Member

@ePaul ePaul Nov 17, 2021

Choose a reason for hiding this comment

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

I guess I missed this on my first round (or these methods were not yet there?).
Is there a reason we pass a map here instead of just one data type and a collection of DTOs?
For all data event types I know, the data_type attribute is effectively constant (and/or ignored).

Do you have examples where regularly different values of data_type are sent on the same event type?
If not, I would go for the simpler signature:

Suggested change
* @param dataTypeToData
* the content of the {@code data_type} field of the Nakadi
* event mapped to some POJOs that can be serialized into JSON (required
* parameter). This is meant to be a representation of the
* current state of the resource. It will be used as content of
* the {@code data} field of the Nakadi event.
*/
@Transactional
void fireCreateEvents(String eventType, Map<String, Collection<Object>> dataTypeToData);
* @param dataType
* the content of the {@code data_type} field of the Nakadi
* event
* @param data
* some POJOs that can be serialized into JSON (required
* parameter). This is meant to be a representation of the
* current state of the resource. It will be used as content of
* the {@code data} field of the Nakadi event.
*/
@Transactional
void fireCreateEvents(String eventType, String dataType, Collection<Object> data);

(And analogously for the other methods.)

Copy link
Member

@ePaul ePaul Nov 22, 2022

Choose a reason for hiding this comment

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

Could you do the same change for the other fire methods, and also the example in the README?

ePaul added a commit that referenced this pull request Nov 26, 2021
@ePaul ePaul added persistence everything around DB access spring-boot-2 Issues/PRs which only apply to the Spring-Boot 2 versions (Releases 20.*+) labels Nov 26, 2021
@ePaul

This comment was marked as resolved.

@ePaul ePaul added the release-21.0 issues/PRs planned for the next major release label Nov 16, 2022
@fbrns fbrns changed the title #140 Persist multiple events in batch [WORK IN PROGRESS] #140 Persist multiple events in batch Nov 23, 2022
@fbrns fbrns changed the title [WORK IN PROGRESS] #140 Persist multiple events in batch #140 Persist multiple events in batch Nov 23, 2022
Comment on lines 26 to 36
@Mock
private EventLogRepository eventLogRepository;
@Mock
private EventLogRepository eventLogRepository;
Copy link
Member

@ePaul ePaul Nov 25, 2022

Choose a reason for hiding this comment

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

In this class the indentation was changed from 4 to 2. Could you please undo this? It makes seeing the actual change quite difficult.
Similarly, in some other files the indentation for new vs. old code doesn't match.

Copy link
Member

Choose a reason for hiding this comment

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

Except from this, it looks good. 👍

fbrns and others added 2 commits November 28, 2022 10:03
This makes it easy to pass any existing collection of a specific type to it,
without an unsafe cast or an additional mapping.
* the {@code data} field of the Nakadi event.
*/
@Transactional
void fireCreateEvents(String eventType, String dataType, Collection<?> data);
Copy link
Member

@ePaul ePaul Nov 28, 2022

Choose a reason for hiding this comment

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

@fbrns I've just changed the type of data to Collection<?> from Collection<Object> (for all the plural fire methods).

This makes it easier to put e.g. a List<DomainObject> into it, without having to unpack and repack it first, or do an unsafe cast (and as we don't actually write anything into this collection here, it's not needed to have a concrete type).

(I actually wanted to create a PR to your branch, but accidentally pushed directly to your branch.)

@ePaul
Copy link
Member

ePaul commented Nov 29, 2022

👍

1 similar comment
@fbrns
Copy link
Member Author

fbrns commented Nov 29, 2022

👍

@ePaul ePaul merged commit 7c6b572 into zalando-nakadi:master Nov 29, 2022
@ePaul ePaul mentioned this pull request Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
persistence everything around DB access release-21.0 issues/PRs planned for the next major release spring-boot-2 Issues/PRs which only apply to the Spring-Boot 2 versions (Releases 20.*+)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants