(6/6) Clean up technical debt in P2PDataStorage and ProtectedStorageEntry objects #3747
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
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.
Now that the only user is internal, the API can be made private and the tests can be removed. This involved adding a few test cases to processGetDataResponse to ensure the invalid hash size condition was still covered.
Now that more callers have moved internal, the public facing API can be cleaner and more simple. This should lead to a more maintainable API and less sharp edges with future development work.
In proto3 the initialized value is an empty ByteString and there are no valid uses of passing in null here.
The ProtectedStoragePayload.fromProto code will throw an exception if this is null from the wire so there is no valid use for it to be null.
In proto3 this is intialized to an empty ByteString so there is no valid use for it to be null.
It is never changed
Helps readability when the variable name matches the type.
Before refactoring the function ensure the tests cover all cases. This fixes a bug where the payload ttl was too low in some instances causing backDate to do no work when it should.
1. Remove delete during stream iteration 2. Minimize branching w/ early returns for bad states 3. Use stream filter for readability 4. Implement additional checks that should be done when removing entries
We already have a garbage collection thread that runs every minute to clean up items. Doing it again during onDisconnect is an unnecessary optimization that adds complexity and caused bugs. For example, the original implementation did not handle the sequence number map correctly and was removing entries during a stream iteration. This also reduces the complexity of testing. There is one code path responsible for reducing ttls and one code path responsible for expiring entries. Much easier to reason about.
ProtectedStorageEntry::backDate() already handles this
julianknutsen
force-pushed
the
clean-up-pse-objs
branch
from
46ebb1f
to
7b8d346
Compare
freimair
suggested changes
Dec 6, 2019
p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java
Show resolved
Hide resolved
@freimair Could you please add how you tested this code changes, so it easier to compare with other ACKs. Thanks! |
ripcurlx
reviewed
Dec 9, 2019
| if (closeConnectionReason.isIntended) | ||
| return; | ||
|
|
||
| if (!connection.getPeersNodeAddressOptional().isPresent()) |
There was a problem hiding this comment.
Is there a reason why you prefer using directly getPeersNodeAddressOptional().isPresent() in comparison using hasPeerNodeAddress() ? It is used mixed in the original code as well, so we might use one way for the future.
There was a problem hiding this comment.
I wish there was a better reason than this one, but I used this pattern because it reduces the number of lint errors from IDEA. If you don't have an isPresent() call prior to a get() it will highlight the word and raise an alert. You can suppress it, but that is just more code to add.
The other benefit is that mocking is bit less error-prone because you just need to mock out the Optional and not the wrapper class that has the hasPeerNodeAddress() function.
ripcurlx
reviewed
Dec 9, 2019
ripcurlx
previously approved these changes
Dec 9, 2019
s/change/chance/
|
@ripcurlx This PR is in a strange GitHub state now. It has the "waiting for author" tag, but since it was ACKd and not "Request Changes" I don't have a button to push to move it back to your queue and can't remove the label myself. Not sure the right way to handle this type of workflow, but I wanted to give you a heads up. In any event, I've addressed your comments and it is ready for another look. |
It was probably not perfectly handled by myself, as I probably should have asked for change requested as status. In that case I was ok about everything, just had optional remarks. Labels can only be changed by maintainers or triage permission owners. I'll have a look how to do it with GitHub actions automatically based on the PR state. |
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.
b6b0026
Motivation
This PR cleans up technical debt in the P2PDataStorage and the ProtectedStorageEntry objects that should be done before new features are added.
Future patches will change these objects quite a bit and getting them into a state that is easy to reason about will make the reviews easier.
Testing
All existing unit tests continue to pass