EventSeq in Typed EventAdapter, #26909 #27130

patriknw · 2019-06-13T06:51:00Z

Tests for usage of EventSeq
Moved event adapter tests from EventSourcedBehaviorSpec to
new EventSourcedEventAdapterSpec
Also support for the event adapter manifest
Note in migration guide

patriknw · 2019-06-13T06:51:24Z

...sistence-typed/src/test/scala/akka/persistence/typed/scaladsl/EventSourcedBehaviorSpec.scala

@@ -484,70 +475,6 @@ class EventSourcedBehaviorSpec extends ScalaTestWithActorTestKit(EventSourcedBeh
      events shouldEqual List(EventEnvelope(Sequence(1), pid.id, 1, Incremented(1)))
    }

-    "adapt events" in {


moved to other test

patriknw · 2019-06-13T06:51:46Z

...ence-typed/src/test/scala/akka/persistence/typed/scaladsl/EventSourcedEventAdapterSpec.scala

+      probe.expectMessage("A1BCD3e")
+    }
+
+    "adapt events" in {


below were existing tests that moved here

patriknw · 2019-06-13T06:54:10Z

akka-persistence-typed/src/main/scala/akka/persistence/typed/EventAdapter.scala

+}
+
+/** INTERNAL API */
+@InternalApi private[akka] final case class SingleEventSeq[A](event: A) extends EventSeq[A] {


tried to make this AnyVal but not possible since extends EventSeq

akka-ci · 2019-06-13T07:29:08Z

Test FAILed.

akka-ci · 2019-06-13T11:40:02Z

Test PASSed.

akka-ci · 2019-07-02T11:22:36Z

Test PASSed.

akka-ci · 2019-07-02T12:33:24Z

Test PASSed.

* Tests for usage of EventSeq * Moved event adapter tests from EventSourcedBehaviorSpec to new EventSourcedEventAdapterSpec * Also support for the event adapter manifest * Note in migration guide

akka-ci · 2019-07-03T10:39:11Z

Test PASSed.

raboof

The EventSeq part itself looks great.

The addition of the manifest seems like a good idea, but perhaps it would be good to have a motivating example in the tests?

raboof · 2019-07-04T14:45:17Z

akka-persistence-typed/src/main/scala/akka/persistence/typed/EventAdapter.scala

+ * <ul>
+ *   <li>extracting events from "envelopes"</li>
+ *   <li>manually converting to the Journals storage format, such as JSON, BSON or any specialised binary format</li>
+ *   <li>adapting incoming events from a "data model" to the "domain model"</li>


Great to make this more clear!

It looks like we're describing this from the toJournal perspective, so perhaps:

Suggested change

* <li>adapting incoming events from a "data model" to the "domain model"</li>

* <li>adapting events from a "domain model" to the "data model"</li>

?

it's supposed to describe what it can be used for, in both directions, clarified somewhat in 83e2bbb

raboof · 2019-07-04T15:01:08Z

akka-persistence/src/main/scala/akka/persistence/Persistent.scala

   * Not used in new records stored with Akka v2.4, but
   * old records from v2.3 may have this as `true` if
   * it was a non-permanent delete.
   */
  def deleted: Boolean

  /**
-   * Sender of this message.
+   * Not used, can be `null`


created issue #27278

raboof · 2019-07-04T15:01:19Z

akka-persistence/src/main/scala/akka/persistence/Persistent.scala

   */
  def withManifest(manifest: String): PersistentRepr

  /**
+   * Not used, can always be `false`.


raboof · 2019-07-04T15:06:29Z

akka-persistence-typed/src/main/scala/akka/persistence/typed/internal/ReplayingEvents.scala

            this
          } catch {
            case NonFatal(ex) =>
              state = state.copy(repr.sequenceNr)
-              onRecoveryFailure(ex, Some(event))
+              onRecoveryFailure(ex, Option(eventForErrorReporting.getOrElse(null)))


Suggested change

onRecoveryFailure(ex, Option(eventForErrorReporting.getOrElse(null)))

onRecoveryFailure(ex, Option(eventForErrorReporting.orNull))

(might be neat to have an toOption on OptionVal?)

added in 83e2bbb

raboof · 2019-07-04T16:16:33Z

...ence-typed/src/test/scala/akka/persistence/typed/scaladsl/EventSourcedEventAdapterSpec.scala

+import com.typesafe.config.ConfigFactory
+import org.scalatest.WordSpecLike
+
+object EventSourcedEventAdapterSpec {


Nice to separate those tests

raboof · 2019-07-04T16:20:08Z

...ence-typed/src/test/scala/akka/persistence/typed/scaladsl/EventSourcedEventAdapterSpec.scala

+    override def toJournal(e: String): String = e.toUpperCase()
+
+    override def fromJournal(p: String, manifest: String): EventSeq[String] = {
+      EventSeq.single(p + manifest)


Heh, that's a nice hack to be able to verify the manifest was retained... would be nicer to have a test where the manifest is used for something 'real', though.

I added the manifest because it is supported in Classic and backend journals support it. It has the same purpose as string manifest in serializers. Leaving it as this here because I don't think it's an important feature.

chbatey

LGTM

akka-ci · 2019-07-05T07:42:12Z

Test PASSed.

akka-ci added the validating PR is currently being validated by Jenkins label Jun 13, 2019

patriknw commented Jun 13, 2019

View reviewed changes

akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Jun 13, 2019

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Jun 13, 2019

patriknw force-pushed the wip-26909-evt-adapter-patriknw branch from 05d67b7 to f72d082 Compare July 2, 2019 10:04

patriknw added the 2 - pick next Used to mark issues which are next up in the queue to be worked on. The tag is non-binding label Jul 2, 2019

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Jul 2, 2019

patriknw added 2 commits July 3, 2019 11:58

EventSeq in Typed EventAdapter, #26909

318bdd9

* Tests for usage of EventSeq * Moved event adapter tests from EventSourcedBehaviorSpec to new EventSourcedEventAdapterSpec * Also support for the event adapter manifest * Note in migration guide

fix Scala 2.13 compiler error

decf338

patriknw force-pushed the wip-26909-evt-adapter-patriknw branch from adc5251 to decf338 Compare July 3, 2019 09:59

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Jul 3, 2019

raboof requested review from raboof and chbatey July 4, 2019 14:11

raboof approved these changes Jul 4, 2019

View reviewed changes

chbatey approved these changes Jul 4, 2019

View reviewed changes

feedback

83e2bbb

akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Jul 5, 2019

raboof approved these changes Jul 5, 2019

View reviewed changes

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Jul 5, 2019

raboof merged commit 72680e9 into master Jul 5, 2019

raboof deleted the wip-26909-evt-adapter-patriknw branch July 5, 2019 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EventSeq in Typed EventAdapter, #26909 #27130

EventSeq in Typed EventAdapter, #26909 #27130

patriknw commented Jun 13, 2019

patriknw Jun 13, 2019

patriknw Jun 13, 2019

patriknw Jun 13, 2019

akka-ci commented Jun 13, 2019

akka-ci commented Jun 13, 2019

akka-ci commented Jul 2, 2019

akka-ci commented Jul 2, 2019

akka-ci commented Jul 3, 2019

raboof left a comment

raboof Jul 4, 2019

patriknw Jul 5, 2019

raboof Jul 4, 2019

patriknw Jul 5, 2019

raboof Jul 4, 2019

raboof Jul 4, 2019

patriknw Jul 5, 2019

raboof Jul 4, 2019

raboof Jul 4, 2019

patriknw Jul 5, 2019

chbatey left a comment

akka-ci commented Jul 5, 2019

	* <li>adapting incoming events from a "data model" to the "domain model"</li>
	* <li>adapting events from a "domain model" to the "data model"</li>

	onRecoveryFailure(ex, Option(eventForErrorReporting.getOrElse(null)))
	onRecoveryFailure(ex, Option(eventForErrorReporting.orNull))

EventSeq in Typed EventAdapter, #26909 #27130

EventSeq in Typed EventAdapter, #26909 #27130

Conversation

patriknw commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Jun 13, 2019

akka-ci commented Jun 13, 2019

akka-ci commented Jul 2, 2019

akka-ci commented Jul 2, 2019

akka-ci commented Jul 3, 2019

raboof 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

chbatey left a comment

Choose a reason for hiding this comment

akka-ci commented Jul 5, 2019