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

Non-reflection eventsourced entity handler #163

Conversation

raboof
Copy link
Contributor

@raboof raboof commented Jul 20, 2021

Based on #150 but excluding the move of the deserialization from the
generated code for now: we might still want to do that, but I'm not
entirely confident about it yet - let's see how JSON support and
testkit re-use pan out.

@raboof raboof force-pushed the wip-non-reflection-event-sourced-take-3 branch 2 times, most recently from d96abf8 to a3d8e59 Compare July 20, 2021 14:13
@raboof raboof force-pushed the wip-non-reflection-event-sourced-take-3 branch from a3d8e59 to 2de27a8 Compare July 21, 2021 10:07
rebase mistake
Copy link
Member

@octonato octonato left a comment

Choose a reason for hiding this comment

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

@raboof, I did a first pass. I will continue tomorrow.

Comment on lines +46 to +47
// ugh, remove
var snapshotEvery = 0
Copy link
Member

Choose a reason for hiding this comment

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

ugh, remove? why?

If it stays, should be private var, no?

Comment on lines +60 to +61
final protected def setState(newState: S): Unit =
state = Some(newState)
Copy link
Member

Choose a reason for hiding this comment

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

Since setState is called with the return of an event handler, we can expect users to return null.

Suggested change
final protected def setState(newState: S): Unit =
state = Some(newState)
final protected def setState(newState: S): Unit =
state = Option(newState)

Comment on lines +54 to +55
require(emptyState != null, "Entity empty state is not allowed to be null")
state = Some(emptyState)
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be allowed to be null. I would expect java developers to define their emptyState with null.

Copy link
Member

Choose a reason for hiding this comment

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

I see that this comes from previous code and only moved to here. Still, I do think that the user can always define its state as null and at any point of its lifecycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I agree - but let's keep it for a different PR given this is the same as before?

Copy link
Member

Choose a reason for hiding this comment

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

fine, I will create an issue about this

currentSequence += 1
shouldSnapshot = shouldSnapshot || (snapshotEvery > 0 && currentSequence % snapshotEvery == 0)
}
// FIXME currently snapshotting final state after applying all events even if trigger was mid-event stream?
Copy link
Member

Choose a reason for hiding this comment

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

I think this is ok since snapshotting is just an optimization. We don't need to be precise about what was exactly the event that reached the threshold

Comment on lines +100 to +102
val endState = stateOrEmpty()
val snapshot =
if (shouldSnapshot) Option(endState)
Copy link
Member

Choose a reason for hiding this comment

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

If we accept that the state can be null, then we don't need to call stateOrEmpty and we can just return state since it's already an Option.

Copy link
Member

@octonato octonato left a comment

Choose a reason for hiding this comment

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

@raboof, there are still some small things to address, like removing the FIXME and making the snapshotEvery private var.

Feel free to merge and address it in follow-up PRs if you prefer.

@raboof raboof merged commit 43f77d6 into lightbend:sdk-codegen-dev Jul 22, 2021
franciscolopezsancho pushed a commit to franciscolopezsancho/kalix-jvm-sdk that referenced this pull request Jul 22, 2021
* Handcoded entity command and event routing, refactored wrapping handler

* Throw EntityException when command type not found

* Fix import

rebase mistake

Co-authored-by: Johan Andrén <johan@markatta.com>
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.

None yet

3 participants