tests: add a test of idempotency of same txn storage

[icehaunter]

Closes #1505

[balegas]

@icehaunter see #1505 (comment)

[icehaunter]

@balegas This adds the aforementioned test. As to whether we should avoid writing at all in case of duplication, I think we can loop back to that question later because under current implementations it's not necessary. If our storage engine becomes actually append-log, then it might become an important idea

[balegas]

Do you mind leaving a comment in the code noting that the way we write to storage actually implies idempotency and why it is required?

I suppose this is remains true for an in-memory approach

[alco]

The code looks good but I have some thoughts on whether it addresses the issue.

Right now the replication stream is only responsible for saving incoming transactions to log storage. But we've already had discussions about streaming transactions in parallel with persisting them to log storage in some cases. As the system evolves, at some point we may replace the current transaction_received callback passed to ReplicationClient with another function potentially from another module, side-stepping the test that only verifies the behaviour of ShapeLogCollector.

I think we can potentially address the duplicate transaction processing problem at the source, i.e. by having information inside ReplicationClient about which lsn it processed last. It's just that implementing this approach requires some design work first.

[balegas]

I see this being merged and the original issue being closed, but I feel that the PR misses the point of the issue.
Ideally the test should cover the any implementation rather than a specific one, but if it doesn't we need to document the assumption, because this implicit property is required for the correctness of the system. I think that is helpful that the test covers any changes that people might do to the current implementation, but I feel that documentation is needed for someone that is making a new storage implementation.

I don't think we need to do the engineering ahead of switching implementations and we might actually touch that code when we do #1506. So, I'm happy addressing the issue at the source there. However, I strongly think that we need to document in code the implicit property.

[alco]

I have filed a new issue specifically for addressing the storage of replication state for the purpose of detecting repeat transactions without involving the shape storage subsystem - #1515.

[icehaunter]

@balegas I've added a storage-level test explicitly for this reason, so that it's less tied to the shape log collector implementation. If someone adds an append-only storage, I would assume they would look at the failing test and question the assumption explicitly stated there. Do you feel like that's not enough?