Fix JournalSpec: AtomicWrite(Seq) instead of Seq(AtomicWrite)

[t3hnar]

Would be nice to have it in 2.4.0

[akka-ci]

Can one of the repo owners verify this patch?

[ktoso]

OK TO TEST

[akka-ci]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3501/

[patriknw]

I think those two if-else branches were accidentally interchanged. It looks like the test data for persistAll is located in the else branch, but it should be here.

[t3hnar]

I think I've just massed up with closing curly braces of map { i => ... }, moment :)

[patriknw]

I mean that the original code was wrong. The fix is to swap places of the if-else

[t3hnar]

@patriknw the problem that in both cases of if-else we have Seq of AtomicWrite with same persistenceId, and none with AtomicWrite of Seq

[patriknw]

@t3hnar isn't this an AtomicWrite(Seq)) ?
I think that was intended to test the persistAll scenario, unless the journal doesn't support it by setting supportsAtomicPersistAllOfSeveralEvents = false

[ktoso]

Yes, this is still wrong;
The first case should be List(AtomicWrite(1,2,3,...)) the second case should be List(AtomicWrite(1), AtomicWrite(2), AtomicWrite(3), ...).

The intent is to allow journals to implement handling of AtomicWrite(1,2,3) by throwing since there's no way it can guarantee it. Such journal should still pass, while it should explicitly document it is not able to provide atomic writes of more than 1 event.

// Yes, it could "store them as one event internally" but we're want to leave this up to the implementation if it does some hack, or says it can not handle this.

[t3hnar]

yes, it is Seq of AtomicWrite of Seq... weird, is it an expected behaviour to send
Seq(AtomicWrite(samePersistenceId, xs), AtomicWrite(samePersistenceId, ys)) instead of
Seq(AtomicWrite(samePersistenceId, xs ++ yx))

[ktoso]

Yes because they are not semantically equivalent. It's expected if you use persist(), only persistAll() will wrap many events into an Atomic op.

It's about the guarantees which are expected from these calls - "All" is the atomic one, and atomic ops are opt-in, we never make them up for the user (the assumption being that they're expensive)

[t3hnar]

@patriknw @ktoso are you going to fix the second case ?

[ktoso]

Since you're in here it would be great if you could pull it through :-)
Let me know if there's something unclear still, happy to help out.

[t3hnar]

ok, ok, will fix both cases :)

[patriknw]

There is some misunderstanding and it can be on my end. There should not be any AtomicWrite(List(1,2,3)) when supportsAtomicPersistAllOfSeveralEvents == false.

Why can't we just swap the cases?

val msgs =
  if (supportsAtomicPersistAllOfSeveralEvents)
    (fromSnr to toSnr - 1).map { i ⇒
      if (i == toSnr - 1)
        AtomicWrite(List(
          PersistentRepr(payload = s"a-$i", sequenceNr = i, persistenceId = pid, sender = sender,
            writerUuid = writerUuid),
          PersistentRepr(payload = s"a-${i + 1}", sequenceNr = i + 1, persistenceId = pid, sender = sender,
            writerUuid = writerUuid)))
      else
        AtomicWrite(PersistentRepr(payload = s"a-${i}", sequenceNr = i, persistenceId = pid, sender = sender,
          writerUuid = writerUuid))
    }

  else
    (fromSnr to toSnr).map { i ⇒
      AtomicWrite(PersistentRepr(payload = s"a-$i", sequenceNr = i, persistenceId = pid, sender = sender,
        writerUuid = writerUuid))
    }

Note that in the supportsAtomicPersistAllOfSeveralEvents == true case I would like to have a mix of things, i.e.
List(AtomicWrite(1), AtomicWrite(2), AtomicWrite(3, 4))

[akka-ci]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3502/

[akka-ci]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3552/

[akka-ci]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3553/

[patriknw]

LGTM, thanks for the cleanup