support handling multiple events #363

ido-namely · 2021-05-24T07:38:42Z

resolves #259:

Call handleEvent for each event in events field.
fallback to event if events is empty.

Make sure to merge proto change pr before merging this PR.

codecov · 2021-05-24T07:44:37Z

Codecov Report

Merging #363 (dad125c) into master (ade4b21) will increase coverage by 1.65%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #363      +/-   ##
==========================================
+ Coverage   94.63%   96.28%   +1.65%     
==========================================
  Files          37       15      -22     
  Lines         745      404     -341     
  Branches       15        8       -7     
==========================================
- Hits          705      389     -316     
+ Misses         40       15      -25

Impacted Files	Coverage Δ
.../scala/com/namely/chiefofstate/AggregateRoot.scala	`95.77% <100.00%> (+0.46%)`	⬆️
...ely/chiefofstate/serialization/CosSerializer.scala
...m/namely/chiefofstate/ServiceMigrationRunner.scala
...ely/chiefofstate/readside/ReadSideProjection.scala
...la/com/namely/chiefofstate/config/BootConfig.scala
...m/namely/chiefofstate/config/WriteSideConfig.scala
.../com/namely/chiefofstate/config/EventsConfig.scala
...om/namely/chiefofstate/config/ReadSideConfig.scala
...namely/chiefofstate/readside/ReadSideHandler.scala
...y/chiefofstate/config/TelemetryConfigFactory.scala
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ade4b21...dad125c. Read the comment docs.

AntonioYuen

Please set your IDE to automatically convert tabs to spaces. I see a few tabs that should be spaces.

AntonioYuen · 2021-05-24T15:48:21Z

code/service/src/main/scala/com/namely/chiefofstate/AggregateRoot.scala

+          response.events.isEmpty match {
+            case true =>
+              response.event match {
+                case Some(newEvent) => Seq(newEvent)
+                case None           => Seq.empty[any.Any]
+              }
+            case _ => response.events
+          })


Suggested change

response.events.isEmpty match {

case true =>

response.event match {

case Some(newEvent) => Seq(newEvent)

case None => Seq.empty[any.Any]

}

case _ => response.events

})

if (response.events.isEmpty) {

response.event match {

case Some(newEvent) => Seq(newEvent)

case None => Seq.empty[any.Any]

}

} else {

response.events

})

Though I am kind of curious here, is i t possible for events and event to both be populated? If so, do we want to preserve both events and event?

My preference is that we use events unless it is not populated.
I stated it in the proto change https://github.com/namely/chief-of-state-protos/pull/23/files

I would actually fail if both are set.

AntonioYuen · 2021-05-24T17:58:34Z

code/service/src/main/scala/com/namely/chiefofstate/AggregateRoot.scala

+              }
+            case _ => response.events
+          })
+        .map(_.filter(x => x != any.Any()) // ignore empty events and validate types


Suggested change

.map(_.filter(x => x != any.Any()) // ignore empty events and validate types

.map(_.filter(_ != any.Any.defaultInstance) // ignore empty Google Any

AntonioYuen · 2021-05-24T18:05:32Z

code/service/src/main/scala/com/namely/chiefofstate/AggregateRoot.scala

+        .map(x =>
+          x match {
+            case None           => WriteHandlerHelpers.NoOp
+            case Some(newState) => newState
+          })


Suggested change

.map(x =>

x match {

case None => WriteHandlerHelpers.NoOp

case Some(newState) => newState

})

.map {

case None => WriteHandlerHelpers.NoOp

case Some(newState) => newState

}

put the parentheses tho

AntonioYuen · 2021-05-24T18:07:09Z

code/service/src/main/scala/com/namely/chiefofstate/AggregateRoot.scala

-      .flatMap({
-        case WriteHandlerHelpers.NewEvent(newEvent) =>
+    // state will be updated with the resulting state of each HandleEventResponse
+    var state = priorState.getState


I would remove this line as it is an anti-pattern in scala.

what would you suggest to do instead? what exactly is the anti pattern?

You're mutating the "state" as you're running your code without a real reason to do so. In functional programming, you typically want to pass around immutable objects.

AntonioYuen · 2021-05-24T18:12:15Z

code/service/src/main/scala/com/namely/chiefofstate/AggregateRoot.scala

          val newEventMeta: MetaData = MetaData()
            .withRevisionNumber(priorState.getMeta.revisionNumber + 1)
            .withRevisionDate(Instant.now().toTimestamp)
            .withData(data)
            .withEntityId(priorState.getMeta.entityId)
            .withHeaders(command.persistedHeaders)

-          val priorStateAny: com.google.protobuf.any.Any = priorState.getState
+          val priorStateAny: com.google.protobuf.any.Any = state


Suggested change

val priorStateAny: com.google.protobuf.any.Any = state

val priorStateAny: com.google.protobuf.any.Any = priorState.getState

AntonioYuen · 2021-05-24T18:12:40Z

code/service/src/main/scala/com/namely/chiefofstate/AggregateRoot.scala

+              state = response.getResultingState
+              WriteHandlerHelpers.NewState(newEvent, state, newEventMeta)


Suggested change

state = response.getResultingState

WriteHandlerHelpers.NewState(newEvent, state, newEventMeta)

WriteHandlerHelpers.NewState(newEvent, response.getResultingState, newEventMeta)

AntonioYuen · 2021-05-24T18:15:32Z

code/service/src/main/scala/com/namely/chiefofstate/AggregateRoot.scala

-
+        })
+	// extract WriteTransitions of each Try or fail, then get the last one (if any exists)
+	.map(_.get).lastOption)


Suggested change

.map(_.get).lastOption)

.map(_.get).lastOption

)

wasn't that supposed to get auto formatted or something?

I don't believe there's an auto format option for where parenthesis end. I just figure it would be much easier to understand this snippet since the ) has nothing to do with that line.

AntonioYuen · 2021-05-24T18:16:49Z

code/service/src/main/scala/com/namely/chiefofstate/AggregateRoot.scala

+            case None           => WriteHandlerHelpers.NoOp
+            case Some(newState) => newState
+          })
+        .recoverWith(makeFailedStatusPf)
    handlerOutput match {


I would add a line break above.

AntonioYuen · 2021-05-24T18:20:21Z

Another thing I noticed is that you're calling .map multiple times. I don't believe there's a reason to not just combine your multiple .maps into one .map

For instance, your handleRemoteCommand could be written like this

  def handleRemoteCommand(
      priorState: StateWrapper,
      command: RemoteCommand,
      replyTo: ActorRef[CommandReply],
      commandHandler: RemoteCommandHandler,
      eventHandler: RemoteEventHandler,
      protosValidator: ProtosValidator,
      data: Map[String, com.google.protobuf.any.Any]): ReplyEffect[EventWrapper, StateWrapper] = {

    val handlerOutput: Try[WriteHandlerHelpers.WriteTransitions] =
      commandHandler
        .handleCommand(command, priorState)
        .map(response => {
          // use events or fallback to event in case events is empty
          val s: Seq[any.Any] = if (response.events.isEmpty) {
            response.event match {
              case Some(newEvent) => Seq(newEvent)
              case None => Seq.empty[any.Any]
            }
          } else {
            response.events
          }

          val filteredEvents: Seq[any.Any] = s
            .filter(_ != any.Any.defaultInstance) // ignore empty events and validate types
            .map(newEvent => {
              protosValidator.requireValidEvent(newEvent)
              newEvent
            })

          // call handleEvent for each event, while passing the updated resulting state from previous call
          val writeTransaction: Option[NewState] = filteredEvents.map(newEvent => {
            val newEventMeta: MetaData = MetaData()
              .withRevisionNumber(priorState.getMeta.revisionNumber + 1)
              .withRevisionDate(Instant.now().toTimestamp)
              .withData(data)
              .withEntityId(priorState.getMeta.entityId)
              .withHeaders(command.persistedHeaders)

            eventHandler
              .handleEvent(newEvent, priorState.getState, newEventMeta)
              .map(response => {
                require(response.resultingState.isDefined, "event handler replied with empty state")
                protosValidator.requireValidState(response.getResultingState)
                WriteHandlerHelpers.NewState(newEvent, response.getResultingState, newEventMeta)
              })
          }).map(_.get).lastOption //extract WriteTransitions of each Try or fail, then get the last one (if any exists)

          writeTransaction.getOrElse(WriteHandlerHelpers.NoOp)
        })
        .recoverWith(makeFailedStatusPf)

    handlerOutput match {
      case Success(NoOp) =>
        Effect.reply(replyTo)(CommandReply().withState(priorState))

      case Success(NewState(event, newState, eventMeta)) =>
        persistEventAndReply(event, newState, eventMeta, replyTo)

      case Failure(e: StatusException) =>
        // record the exception on the current span
        Span.current().recordException(e).setStatus(StatusCode.ERROR)
        // reply with the error status
        Effect.reply(replyTo)(CommandReply().withError(toRpcStatus(e.getStatus, e.getTrailers)))

      case x =>
        // this should never happen, but here for code completeness
        val errStatus = Status.INTERNAL.withDescription(s"write handler failure, ${x.getClass}")

        Span.current().recordException(errStatus.asException()).setStatus(StatusCode.ERROR)
        Effect.reply(replyTo)(CommandReply().withError(toRpcStatus(errStatus)))
    }
  }

ido-namely

Another thing I noticed is that you're calling .map multiple times. I don't believe there's a reason to not just combine your multiple .maps into one .map

For instance, your handleRemoteCommand could be written like this

  def handleRemoteCommand(
      priorState: StateWrapper,
      command: RemoteCommand,
      replyTo: ActorRef[CommandReply],
      commandHandler: RemoteCommandHandler,
      eventHandler: RemoteEventHandler,
      protosValidator: ProtosValidator,
      data: Map[String, com.google.protobuf.any.Any]): ReplyEffect[EventWrapper, StateWrapper] = {

    val handlerOutput: Try[WriteHandlerHelpers.WriteTransitions] =
      commandHandler
        .handleCommand(command, priorState)
        .map(response => {
          // use events or fallback to event in case events is empty
          val s: Seq[any.Any] = if (response.events.isEmpty) {
            response.event match {
              case Some(newEvent) => Seq(newEvent)
              case None => Seq.empty[any.Any]
            }
          } else {
            response.events
          }

          val filteredEvents: Seq[any.Any] = s
            .filter(_ != any.Any.defaultInstance) // ignore empty events and validate types
            .map(newEvent => {
              protosValidator.requireValidEvent(newEvent)
              newEvent
            })

          // call handleEvent for each event, while passing the updated resulting state from previous call
          val writeTransaction: Option[NewState] = filteredEvents.map(newEvent => {
            val newEventMeta: MetaData = MetaData()
              .withRevisionNumber(priorState.getMeta.revisionNumber + 1)
              .withRevisionDate(Instant.now().toTimestamp)
              .withData(data)
              .withEntityId(priorState.getMeta.entityId)
              .withHeaders(command.persistedHeaders)

            eventHandler
              .handleEvent(newEvent, priorState.getState, newEventMeta)
              .map(response => {
                require(response.resultingState.isDefined, "event handler replied with empty state")
                protosValidator.requireValidState(response.getResultingState)
                WriteHandlerHelpers.NewState(newEvent, response.getResultingState, newEventMeta)
              })
          }).map(_.get).lastOption //extract WriteTransitions of each Try or fail, then get the last one (if any exists)

          writeTransaction.getOrElse(WriteHandlerHelpers.NoOp)
        })
        .recoverWith(makeFailedStatusPf)

    handlerOutput match {
      case Success(NoOp) =>
        Effect.reply(replyTo)(CommandReply().withState(priorState))

      case Success(NewState(event, newState, eventMeta)) =>
        persistEventAndReply(event, newState, eventMeta, replyTo)

      case Failure(e: StatusException) =>
        // record the exception on the current span
        Span.current().recordException(e).setStatus(StatusCode.ERROR)
        // reply with the error status
        Effect.reply(replyTo)(CommandReply().withError(toRpcStatus(e.getStatus, e.getTrailers)))

      case x =>
        // this should never happen, but here for code completeness
        val errStatus = Status.INTERNAL.withDescription(s"write handler failure, ${x.getClass}")

        Span.current().recordException(errStatus.asException()).setStatus(StatusCode.ERROR)
        Effect.reply(replyTo)(CommandReply().withError(toRpcStatus(errStatus)))
    }
  }

but we still have to call map on the Seq twice, so what we are saving here is the map call on the Try?
Does that make a difference?

ido-namely · 2021-05-24T21:21:41Z

code/service/src/main/scala/com/namely/chiefofstate/AggregateRoot.scala

-      .flatMap({
-        case WriteHandlerHelpers.NewEvent(newEvent) =>
+    // state will be updated with the resulting state of each HandleEventResponse
+    var state = priorState.getState


what would you suggest to do instead? what exactly is the anti pattern?

ido-namely · 2021-05-24T21:23:04Z

code/service/src/main/scala/com/namely/chiefofstate/AggregateRoot.scala

+          response.events.isEmpty match {
+            case true =>
+              response.event match {
+                case Some(newEvent) => Seq(newEvent)
+                case None           => Seq.empty[any.Any]
+              }
+            case _ => response.events
+          })


My preference is that we use events unless it is not populated.
I stated it in the proto change https://github.com/namely/chief-of-state-protos/pull/23/files

ido-namely · 2021-05-24T21:24:45Z

code/service/src/main/scala/com/namely/chiefofstate/AggregateRoot.scala

-
+        })
+	// extract WriteTransitions of each Try or fail, then get the last one (if any exists)
+	.map(_.get).lastOption)


wasn't that supposed to get auto formatted or something?

AntonioYuen · 2021-05-24T23:05:48Z

but we still have to call map on the Seq twice, so what we are saving here is the map call on the Try?
Does that make a difference?

In my opinion, it is much more pleasant to read if you're not calling .map multiple times. For instance, this line is unpleasant and it is hard to tell where it ends (on line 223).

I feel like your thought process by doing this is to physically segregate your transformations. If that's the case I would actually write out the mini functions where they're testable and plug them in directly into your map function. It allows for better unit tests.

ido-namely added 2 commits May 24, 2021 03:30

support handling multiple events

953f761

update comment

3ccac7a

ido-namely requested review from klvmungai, Tochemey and zenyui as code owners May 24, 2021 07:38

Merge branch 'master' into ido-multiple-events-per-command

daac94f

AntonioYuen suggested changes May 24, 2021

View reviewed changes

AntonioYuen reviewed May 24, 2021

View reviewed changes

ido-namely commented May 24, 2021

View reviewed changes

Tochemey added 4 commits May 25, 2021 19:27

Merge branch 'master' into ido-multiple-events-per-command

a1ba3c9

Merge branch 'master' into ido-multiple-events-per-command

fce1933

Merge branch 'master' into ido-multiple-events-per-command

4fc09f5

Merge branch 'master' into ido-multiple-events-per-command

dad125c

klvmungai closed this Oct 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support handling multiple events #363

support handling multiple events #363

ido-namely commented May 24, 2021 •

edited

Loading

codecov bot commented May 24, 2021 •

edited

Loading

AntonioYuen left a comment

AntonioYuen May 24, 2021

AntonioYuen May 24, 2021

ido-namely May 24, 2021

zenyui May 24, 2021

AntonioYuen May 24, 2021

AntonioYuen May 24, 2021

zenyui May 24, 2021

AntonioYuen May 24, 2021

ido-namely May 24, 2021

AntonioYuen May 24, 2021

AntonioYuen May 24, 2021

AntonioYuen May 24, 2021

AntonioYuen May 24, 2021 •

edited

Loading

ido-namely May 24, 2021

AntonioYuen May 24, 2021

AntonioYuen May 24, 2021

AntonioYuen commented May 24, 2021 •

edited

Loading

ido-namely left a comment

ido-namely May 24, 2021

ido-namely May 24, 2021

ido-namely May 24, 2021

AntonioYuen commented May 24, 2021 •

edited

Loading

	.map(_.filter(x => x != any.Any()) // ignore empty events and validate types
	.map(_.filter(_ != any.Any.defaultInstance) // ignore empty Google Any

	val priorStateAny: com.google.protobuf.any.Any = state
	val priorStateAny: com.google.protobuf.any.Any = priorState.getState

		state = response.getResultingState
		WriteHandlerHelpers.NewState(newEvent, state, newEventMeta)

	state = response.getResultingState
	WriteHandlerHelpers.NewState(newEvent, state, newEventMeta)
	WriteHandlerHelpers.NewState(newEvent, response.getResultingState, newEventMeta)

support handling multiple events #363

support handling multiple events #363

Conversation

ido-namely commented May 24, 2021 • edited Loading

codecov bot commented May 24, 2021 • edited Loading

Codecov Report

AntonioYuen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntonioYuen May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntonioYuen commented May 24, 2021 • edited Loading

ido-namely left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntonioYuen commented May 24, 2021 • edited Loading

ido-namely commented May 24, 2021 •

edited

Loading

codecov bot commented May 24, 2021 •

edited

Loading

AntonioYuen May 24, 2021 •

edited

Loading

AntonioYuen commented May 24, 2021 •

edited

Loading

AntonioYuen commented May 24, 2021 •

edited

Loading