Fix handle_all_serials for the new and old protocols. #86
Conversation
|
Would anyone like to take a look at this? It's the same fix as #85 (except sniffing is really what we want to do in ZODB 4). |
| elif arg: | ||
| for t in arg: | ||
| if isinstance(t, bytes): | ||
| # This will be the tid returned by tpc_finish. |
There was a problem hiding this comment.
It could also be an oid that's returned by vote to indicate that a conflict was resolved. This is important in some situations, but perhaps not in cases that are tested here.
This is a mess (like a lot of ZODB test infrastructure). For this function to be "correct", it needs to consider where the data came from.
There was a problem hiding this comment.
The right thing to do would be to look at the call points and decide WTF.
Given that this is the 4 branch, I guess I'd be willing to accept this if the tests pass and if we leave an XXX comment here saying that the this should get another review after reviewing the call points.
There was a problem hiding this comment.
On Jul 12, 2016, at 11:11, Jim Fulton notifications@github.com wrote:
# This will be the tid returned by tpc_finish.It could also be an oid that's returned by vote to indicate that a conflict was resolved. This is important in some situations, but perhaps not in cases that are tested here.
This is a mess (like a lot of ZODB test infrastructure). For this function to be "correct", it needs to consider where the data came from.
The comment is unclear. I meant that the caller will wind up using the tid returned by tpc_finish in this case (because the dict will be empty). I can clarify.
There was a problem hiding this comment.
On Jul 12, 2016, at 11:14, Jim Fulton notifications@github.com wrote:
The right thing to do would be to look at the call points and decide WTF.
Given that this is the 4 branch, I guess I'd be willing to accept this if the tests pass and if we leave an XXX comment here saying that the this should get another review after reviewing the call points.
Happily the tests do all pass.
I just reviewed the callers for #87 (in the process of removing them as requested), and they all do exactly the same thing as the test here did:
r1 = self._storage.store(...)
r2 = self._storage.tpc_vote(txn)
serial = self._storage.tpc_finish(txn)
newrevid = handle_serials(oid, r1, r2)
if newrevid is None and serial is not None:
newrevid = serial
For whatever that's worth...
Same as #85 but for ZODB 4.