(1/5) Test and Refactor P2PDataStorage Synchronization Path #3667
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
julianknutsen
changed the title
julianknutsen
commented
Nov 23, 2019
julianknutsen
force-pushed
the
refactor-get-data-smaller
branch
from
2b020a9
to
15226a9
Compare
This will allow us to push the GetData creation inside P2PDataStorage safely.
As part of changing the GetData path, we want to move all creation and processing of GetData messages inside P2PDataStorage. This will allow easier unit testing of the behavior as well as cleaner code in the request handlers that can just focus on nonces, connections, etc.
These are identical test cases to the requestHandler tests, but with much fewer dependencies. The requestHandler tests will eventually be deleted, but they are going to remain throughout development as an extra safety net.
Add some basic sanity tests prior to the refactor to help catch issues.
This is just a strict move of code to reduce errors.
Changed the log to reference getDataResponse instead of getData. Now that we might truncate the response, it ins't true that this is exactly what the peer asked.
Move the logging that utilizes connection information into the request handler. Now, buildGetDataResponse just returns whether or not the list is truncated which will make it easier to test.
Remove the dependence on the connection object by having the handler pass in the peer's capabilities. This now allows unit testing of buildGetDataResponse without any connection dependencies.
Write a full set of unit tests for buildGetDataResponse. This provides a safety net with additional refactoring work.
The appendOnlyDataStoreService and map already have unique keys that
are based on the hash of the payload. This would catch instances
where:
PersistableNetworkPayload
- None: The key is based on ByteArray(payload.getHash()) which is the
same as this check.
ProtectedStorageEntry
- Cases where multiple PSEs contain payloads that have equivalent
hashCode(), but different data.toProtoMessage().toByteArray().
I don't think it is a good idea to keep 2 "unique" methods on
payloads. This is likely left over from a time when
Payload hashCode() needed to be different than the hash of
the payload.
Move the logging function to the common capabilities check so it can run on both ProtectedStoragePayload and PersistableNetworkPayload objects
Move the capability check inside the stream operation. This should improve performance slightly, but more importantly it makes the two filter functions almost identical so they can be combined.
Removes unnecessary calculations converting Set<byte[]> into Set<ByteArray> and allows additional deduplication of stream operations.
Introduce a generic function that can be used to filter Map<ByteArray, PersistableNetworkPayload> or Map<ByteArray, ProtectedStorageEntry>. Used to deduplicate the GetData code paths and ensure the logic is the same between the two payload types.
Now, the truncation is only triggered if more than MAX_ENTRIES could have been returned.
Add heavy-handed test that exercises the logic to use as a safeguard for refactoring.
Just a code move for now.
Now that we want to unit test the GetData path which has different behavior w.r.t. broadcasts, the tests need a way to verify that state was updated, but not broadcast during an add. This patch changes all verification function to take each state update explicitly so the tests can do the proper verification.
Add a full set of unit tests that uncovered some unexpected behavior w.r.t. signalers.
Previously, multiple handlers needed to signal off one global variable. Now, that this check is inside the singleton P2PDataStorage, make it non-static and private.
Write a few integration test that exercises the exercise interesting synchronization states including the lost remove bug. This fails with the proper validation, but will pass at the end of the new feature development.
- Add more comments - Use Clock instead of System - Remove unnecessary AtomicInteger
Name is left over from previous implementation. Change it to be more relevant to the current code and update comments to indicate the current usage.
Checking for null creates hard-to-read code and it is simpler to just create an empty set if we receive a pre-v0.6 GetDataResponse protobuf message that does not have the field set.
The only two users of this constructor are the fromProto path which already creates an empty Capabilities object if one is not provided and the internal usage of Capabilities.app which is initialized to empty. Remove the @nullable so future readers aren't confused.
…quest The only two users of this constructor are the fromProto path which now creates an empty Capabilities object similar to GetDataResponse. The other internal usage of Capabilities.app which is initialized to empty.
Now that all the implementations are unit tested in P2PDataStorage, the old tests can be removed.
julianknutsen
force-pushed
the
refactor-get-data-smaller
branch
from
15226a9
to
4fe19ae
Compare
|
Rebased against master and published now that v1.2.4 is branched. |
julianknutsen
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The GetData synchronization sequence was currently untested and the code was tangled between the connection request handling logic and the
P2PDataStorageupdate logic. In order to implement a new feature (replay removes during synchronization) I needed a way to verify current behavior and write tests for the changes I am making.Changes
This PR moves the
GetDataRequest->GetDataResponse->ProcessDataResponselogic into theP2PDataStorage where it can operate locally on the various data structures that need to be accessed
and updated. This has a few benefits:
FutureandConnectionto validate behaviorthe P2PDataStorage handle the adds/removes/updates.
P2PDataStoragelimiting external knowledge of implementation detailsP2PDataStorageinstances so letting the actual objects build and apply the synchronization messages makes more intuitive sense.Tests
Full unit test for each individual stage now exists as well as an integration test that creates two separate
P2PDataStorageinstances, runs the synchronization logic, and verifies they are synchronized. This type of "multiple node" testing will be used to validate the new feature development and will limit errors in development.Maintainability fixes
Since this patch tests the
GetDatamessages sent back and forth, there was an opportunity to clean up the guarantees around null vs. empty w.r.t. sets and Capabilities. I think this is a good pattern moving forward to limit accidental NullPointerExceptions when we can do something less brittle.