-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(6/6) Clean up technical debt in P2PDataStorage and ProtectedStorageEntry objects #3747
(6/6) Clean up technical debt in P2PDataStorage and ProtectedStorageEntry objects #3747
Commits on Dec 3, 2019
-
[TESTS] Add tests of requestData
This will allow us to push the GetData creation inside P2PDataStorage safely.
Configuration menu - View commit details
-
Copy full SHA for 5fcd18c - Browse repository at this point
Copy the full SHA 5fcd18cView commit details -
[REFACTOR] Introduce buildGetDataRequest variants
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.
Configuration menu - View commit details
-
Copy full SHA for 1e814d9 - Browse repository at this point
Copy the full SHA 1e814d9View commit details -
[TESTS] Add tests of new RequestData APIs
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.
Configuration menu - View commit details
-
Copy full SHA for a927ed4 - Browse repository at this point
Copy the full SHA a927ed4View commit details -
[TESTS] Add tests of GetDataRequestHandler
Add some basic sanity tests prior to the refactor to help catch issues.
Configuration menu - View commit details
-
Copy full SHA for daffe6d - Browse repository at this point
Copy the full SHA daffe6dView commit details -
[REFACTOR] Introduce buildGetDataResponse
This is just a strict move of code to reduce errors.
Configuration menu - View commit details
-
Copy full SHA for 944b3ff - Browse repository at this point
Copy the full SHA 944b3ffView commit details -
Configuration menu - View commit details
-
Copy full SHA for 8208f78 - Browse repository at this point
Copy the full SHA 8208f78View commit details -
[REFACTOR] Extract getDataResponse logging
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.
Configuration menu - View commit details
-
Copy full SHA for 5402155 - Browse repository at this point
Copy the full SHA 5402155View commit details -
[REFACTOR] Extract truncation logging
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.
Configuration menu - View commit details
-
Copy full SHA for a6e8868 - Browse repository at this point
Copy the full SHA a6e8868View commit details -
[REFACTOR] Pass peerCapabilities into buildGetDataResponse
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.
Configuration menu - View commit details
-
Copy full SHA for dafc762 - Browse repository at this point
Copy the full SHA dafc762View commit details -
[TESTS] Unit tests of buildGetDataResponse
Write a full set of unit tests for buildGetDataResponse. This provides a safety net with additional refactoring work.
Configuration menu - View commit details
-
Copy full SHA for 5630b35 - Browse repository at this point
Copy the full SHA 5630b35View commit details -
Remove redundant HashSet lookups in filter functions
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.
Configuration menu - View commit details
-
Copy full SHA for caf946d - Browse repository at this point
Copy the full SHA caf946dView commit details -
[REFACTOR] Move required capabilities log
Move the logging function to the common capabilities check so it can run on both ProtectedStoragePayload and PersistableNetworkPayload objects
Configuration menu - View commit details
-
Copy full SHA for 703a9a0 - Browse repository at this point
Copy the full SHA 703a9a0View commit details -
[REFACTOR] Inline capability check for ProtectedStorageEntries
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.
Configuration menu - View commit details
-
Copy full SHA for 3aaf8a2 - Browse repository at this point
Copy the full SHA 3aaf8a2View commit details -
[REFACTOR] Inline filtering functions
Removes unnecessary calculations converting Set<byte[]> into Set<ByteArray> and allows additional deduplication of stream operations.
Configuration menu - View commit details
-
Copy full SHA for 4c5d818 - Browse repository at this point
Copy the full SHA 4c5d818View commit details -
[REFACTOR] Remove duplication in filtering functions
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.
Configuration menu - View commit details
-
Copy full SHA for e767340 - Browse repository at this point
Copy the full SHA e767340View commit details -
[BUGFIX] Fix off-by-one in truncation logic
Now, the truncation is only triggered if more than MAX_ENTRIES could have been returned.
Configuration menu - View commit details
-
Copy full SHA for 00128d9 - Browse repository at this point
Copy the full SHA 00128d9View commit details -
[TESTS] Add test of RequestDataHandler::onMessage
Add heavy-handed test that exercises the logic to use as a safeguard for refactoring.
Configuration menu - View commit details
-
Copy full SHA for c7bce9e - Browse repository at this point
Copy the full SHA c7bce9eView commit details -
[REFACTOR] Introduce processGetDataResponse
Just a code move for now.
Configuration menu - View commit details
-
Copy full SHA for 873271c - Browse repository at this point
Copy the full SHA 873271cView commit details -
[TESTS] Make verify() functions more flexible
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.
Configuration menu - View commit details
-
Copy full SHA for 690b980 - Browse repository at this point
Copy the full SHA 690b980View commit details -
[TESTS] Add unit tests for processGetDataResponse
Add a full set of unit tests that uncovered some unexpected behavior w.r.t. signalers.
Configuration menu - View commit details
-
Copy full SHA for a34488b - Browse repository at this point
Copy the full SHA a34488bView commit details -
Remove static from initialRequestApplied
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.
Configuration menu - View commit details
-
Copy full SHA for 3d6e9fb - Browse repository at this point
Copy the full SHA 3d6e9fbView commit details -
[TESTS] Write synchronization integration tests
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.
Configuration menu - View commit details
-
Copy full SHA for f92893b - Browse repository at this point
Copy the full SHA f92893bView commit details -
[REFACTOR] Clean up processGetDataResponse
- Add more comments - Use Clock instead of System - Remove unnecessary AtomicInteger
Configuration menu - View commit details
-
Copy full SHA for 5db1285 - Browse repository at this point
Copy the full SHA 5db1285View commit details -
[RENAME] LazyProcessedPayload to ProcessOncePersistableNetworkPayload
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.
Configuration menu - View commit details
-
Copy full SHA for ecae31e - Browse repository at this point
Copy the full SHA ecae31eView commit details -
Remove @nullable around persistableNetworkPayloadSet
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.
Configuration menu - View commit details
-
Copy full SHA for a0fae12 - Browse repository at this point
Copy the full SHA a0fae12View commit details -
Remove @nullable around supportedCapabilities in GetDataResponse
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.
Configuration menu - View commit details
-
Copy full SHA for c503bcb - Browse repository at this point
Copy the full SHA c503bcbView commit details -
Remove @nullable around supportedCapabilities in PreliminaryGetDataRe…
…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.
Configuration menu - View commit details
-
Copy full SHA for b1a06fe - Browse repository at this point
Copy the full SHA b1a06feView commit details -
[DEADCODE] Remove old request handler tests
Now that all the implementations are unit tested in P2PDataStorage, the old tests can be removed.
Configuration menu - View commit details
-
Copy full SHA for 4fe19ae - Browse repository at this point
Copy the full SHA 4fe19aeView commit details -
Make addPersistableNetworkPayloadFromInitialRequest private
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.
Configuration menu - View commit details
-
Copy full SHA for 0649323 - Browse repository at this point
Copy the full SHA 0649323View commit details -
[REFACTOR] Clean up ClientAPI for addPersistableNetworkPayload
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.
Configuration menu - View commit details
-
Copy full SHA for 56a7661 - Browse repository at this point
Copy the full SHA 56a7661View commit details -
[REFACTOR] Clean up ClientAPI for addProtectedStorageEntry
Remove isDataOwner from the client API. All users pass in true. All test users don't care.
Configuration menu - View commit details
-
Copy full SHA for 0e6b1a2 - Browse repository at this point
Copy the full SHA 0e6b1a2View commit details -
[REFACTOR] Clean up ClientAPI for remove
Remove isDataOwner from the client API. All users pass in true. All test users don't care.
Configuration menu - View commit details
-
Copy full SHA for 77413c9 - Browse repository at this point
Copy the full SHA 77413c9View commit details -
[REFACTOR] Clean up ClientAPI for refreshTTL
Remove isDataOwner from the client API. All users pass in true. All test users don't care.
Configuration menu - View commit details
-
Copy full SHA for 9f69134 - Browse repository at this point
Copy the full SHA 9f69134View commit details -
Make isDataOwner a private policy decision in BroadcastHandler
isDataOwner is used when deciding how many peer nodes should receive a BroadcastMessage. If the BroadcastMessage originated on the local node it is sent to ALL peer nodes with a small delay. If the node is only relaying the message (it originated on a different node) it is sent to MAX(peers.size(), 7) peers with a delay that is twice as long. All the information needed to determine whether or not the BroadcastMessage originated on the local node is available at the final broadcast site and there is no reason to have callers pass it in. In the event that the sender address is not known during broadcast (which is only a remote possibility due to how early the local node address is set during startup) we can default to relay mode. This first patch just removes the deep parameters. The next will remove everything else. There is one real change in LiteNodeNetworkService.java where it was using the local node when it should have been using the peer node. This was updated to the correct behavior.
Configuration menu - View commit details
-
Copy full SHA for bfdb8f5 - Browse repository at this point
Copy the full SHA bfdb8f5View commit details -
Remove isDataOwner from P2PDataStorage
Remove all usages in code and tests now that the behavior is internal to the BroadcastHandler
Configuration menu - View commit details
-
Copy full SHA for 4dc4532 - Browse repository at this point
Copy the full SHA 4dc4532View commit details -
[REFACTOR] inline broadcast() private function
Now that it is identical the the broadcaster version it can be inlined.
Configuration menu - View commit details
-
Copy full SHA for 6ff8756 - Browse repository at this point
Copy the full SHA 6ff8756View commit details -
[REFACTOR] inline broadcastProtectedStorageEntry() private function
Now that it is identical the the broadcaster version it can be inlined.
Configuration menu - View commit details
-
Copy full SHA for 5a174d5 - Browse repository at this point
Copy the full SHA 5a174d5View commit details -
[REFACTOR] inline maybeAddToRemoveAddOncePayloads() private function
Now that there is only one user it can be inlined.
Configuration menu - View commit details
-
Copy full SHA for 1bd450b - Browse repository at this point
Copy the full SHA 1bd450bView commit details -
[TESTS] Clean up mockito never() and eq(null) usages
never() and any() don't play well together for nullable types. Change the broadcast mocks to user nullable() and fixup tests.
Configuration menu - View commit details
-
Copy full SHA for 17f4b70 - Browse repository at this point
Copy the full SHA 17f4b70View commit details
Commits on Dec 4, 2019
-
Remove ProtectedStorageEntry::updateSignature
The only users were tests that can just pass a bad signature directly into the constructor.
Configuration menu - View commit details
-
Copy full SHA for b6b0026 - Browse repository at this point
Copy the full SHA b6b0026View commit details -
Remove ProtectedStorageEntry::maybeAdjustCreationTimeStamp
There is only 1 caller can be replaced with Math.min()
Configuration menu - View commit details
-
Copy full SHA for 24ecfc7 - Browse repository at this point
Copy the full SHA 24ecfc7View commit details -
@NotNull ProtectedStorageEntry::ownerPubKey
In proto3 the initialized value is an empty ByteString and there are no valid uses of passing in null here.
Configuration menu - View commit details
-
Copy full SHA for 0c67608 - Browse repository at this point
Copy the full SHA 0c67608View commit details -
@NotNull ProtectedStorageEntry::protectedStoragePayload
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.
Configuration menu - View commit details
-
Copy full SHA for 76e8c57 - Browse repository at this point
Copy the full SHA 76e8c57View commit details -
@NotNull MailboxStoragePayload::senderPubKeyForAddOperation
In proto3 this is intialized to an empty ByteString so there is no valid use for it to be null.
Configuration menu - View commit details
-
Copy full SHA for 104984c - Browse repository at this point
Copy the full SHA 104984cView commit details
Commits on Dec 5, 2019
-
Configuration menu - View commit details
-
Copy full SHA for 01a7f79 - Browse repository at this point
Copy the full SHA 01a7f79View commit details -
s/networkPayload/protectedStoragePayload
Helps readability when the variable name matches the type.
Configuration menu - View commit details
-
Copy full SHA for c38ff9b - Browse repository at this point
Copy the full SHA c38ff9bView commit details -
[TESTS] Make onDisconnect tests more robust
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.
Configuration menu - View commit details
-
Copy full SHA for 688405b - Browse repository at this point
Copy the full SHA 688405bView commit details -
Refactor P2PDataStorage::onDisconnect
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
Configuration menu - View commit details
-
Copy full SHA for df2e4cc - Browse repository at this point
Copy the full SHA df2e4ccView commit details -
Remove expire optimization in onDisconnect
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.
Configuration menu - View commit details
-
Copy full SHA for b166009 - Browse repository at this point
Copy the full SHA b166009View commit details -
Remove filter for ExpirablePayload
ProtectedStorageEntry::backDate() already handles this
Configuration menu - View commit details
-
Copy full SHA for 7b8d346 - Browse repository at this point
Copy the full SHA 7b8d346View commit details
Commits on Dec 9, 2019
-
Configuration menu - View commit details
-
Copy full SHA for e8c8225 - Browse repository at this point
Copy the full SHA e8c8225View commit details