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

atLeastOnce parameters #59

Closed
patriknw opened this issue Apr 22, 2020 · 15 comments
Closed

atLeastOnce parameters #59

patriknw opened this issue Apr 22, 2020 · 15 comments
Assignees
Milestone

Comments

@patriknw
Copy link
Member

The CassandraProjection.atLeastOnce maybe has too many parameters to be convenient?
We could remove saveOffsetAfterElements and saveOffsetAfterDuration and place them in configuration.

Then have a builder style api to define them programmatically. atLeastOnce would then return AtLeastOnceCassandraProjection extends CassandraProjection which has the additional withSaveOffsetAfterElements and withSaveOffsetAfterDuration.

CassandraProjection.atLeastOnce(
  projectionId,
  sourceProvider,
  offsetExtractor) { env => ... }
  . withSaveOffsetAfterElements(100)
  . withSaveOffsetAfterDuration(1.second)

Probably not nice with the handler as second parameter list, but we talked about making the handler a trait instead of a function and then it will not be a second parameter list.

@octonato
Copy link
Member

I often prefer a builder approach because fluent APIs are really good for discoverability even when we pay the cost of writing a few inner stage classes.

We can even close the builder with the event handler (function or trait) and builds the final projection.

@seglo
Copy link
Member

seglo commented Apr 22, 2020

If we create a Settings object then they could be more easily reused with multiple projections. That's what's done in Alpakka Kafka. The settings objects have defaults that can be overridden using typesafe config or at runtime using builder-pattern APIs.

@patriknw
Copy link
Member Author

Settings are good for general configuration, but here it was about parameters that are specific to a method, and are not used for other methods. atLeastOnce vs atMostOnce.

@octonato
Copy link
Member

Can we then pass a strategy instance, for example:

CassandraProjection(
  projectionId,
  sourceProvider,
  AtLeastOnce(100, 1.second),
  handler)

and

CassandraProjection(
  projectionId,
  sourceProvider,
  AtMostOnce,
  handler)

@octonato
Copy link
Member

Or maybe we prefer build style?

CassandraProjection(
  projectionId,
  sourceProvider,
  handler).atLeastOnce(100, 1.second)

and

CassandraProjection(
  projectionId,
  sourceProvider,
  handler).atMostOnce

@octonato
Copy link
Member

Now that the handler isn't a function anymore, it doesn't need to be the last in the list, and we can close the builder by declaring how we want to treat the commits.

@patriknw
Copy link
Member Author

I think the builder is not better than what we have. Builders are great for optional parameters, but these are mandatory choices.

I don't think the strategy is easier to use, not as nice for IDE code completion. Additionally, very few will use atMostOnce.

I have some experiments for grouping lying around and I think we need to add that to the mix before deciding here.

@octonato
Copy link
Member

I have some experiments for grouping lying around and I think we need to add that to the mix before deciding here.

Looking forward to it

@patriknw
Copy link
Member Author

Or we move the two parameters for atLeastOnce to config (ProjectionSettings). Don’t think they will tuned much per projection.

@octonato
Copy link
Member

I understood that your idea is that those are mandatory parameters, like method arguments, and therefore should not be on the settings.

Moreover, the settings are good for all projections, not only for at-least-once

@patriknw
Copy link
Member Author

Yes, that was my initial thinking, but it should be possible to have default that normally works and then it can be tuned if needed instead.

@octonato
Copy link
Member

But then in that case we will have at-least-once settings that are exposed at API level, but maybe be useless. Like when using atMostOnce or jdbc exactlyOnce.

@octonato
Copy link
Member

Unless we return a AtLeastOnceProjection that has the methods withSaveOffsetAfterElements and withSaveOffsetAfterDuration in addition to withSettings

@patriknw
Copy link
Member Author

yes, that's nice. Defaults from config and possibility to amend.

@seglo
Copy link
Member

seglo commented May 22, 2020

Implemented in #175

@seglo seglo closed this as completed May 22, 2020
@patriknw patriknw added this to the 0.2.0 milestone Jun 5, 2020
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

No branches or pull requests

3 participants