Add getNodeRecordBuckets, addNodeRecord, and deleteNode to DiscoverySystem#188
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
garyschulte
left a comment
garyschulte
left a comment
There was a problem hiding this comment.
Open question about explicit add/delete. If we were to run plugin code in the same process space it might pose a risk to allow modifying discovery system.
Also, seems like there is a broken (or flaky) integration test
|
Thanks Gary, I'll hold off on merging this until the Teku guys have a chance to take a look too. They're all on leave this week! |
Nashatyrev
left a comment
Nashatyrev
left a comment
There was a problem hiding this comment.
LGTM, just a minor comment
| void addNodeRecord(NodeRecord nodeRecord); | ||
|
|
||
| /** | ||
| * Deletes the NodeRecord identified by nodeId from the routing table | ||
| * | ||
| * @param nodeId The node ID to be deleted from the routing table | ||
| */ | ||
| void deleteNode(Bytes nodeId); |
There was a problem hiding this comment.
Just an idea: I wonder if users of this mutable interface want some feedback on the operation they request? e.g. should we return a bool to indicate a successful operation?
There was a problem hiding this comment.
I don't think it's required for Samba's usecase and it can be added later if desired, so I'll leave it for now.
pinges
left a comment
pinges
left a comment
There was a problem hiding this comment.
LGTM, just wanting to understand why these methods need to be synchronized (I know, it wasn't you who started it), and two suggestions for method names.
| return getNode(nodeId).isPresent(); | ||
| } | ||
|
|
||
| public synchronized List<List<NodeRecord>> getNodeRecordBuckets() { |
There was a problem hiding this comment.
Not sure I like this name. We are not returning the buckets, we are returning node records from all buckets. Maybe a simple name like getNodeRecords() is better?
There was a problem hiding this comment.
I included the words buckets because each sub list is a list of node records from a bucket. I'm not sure getNodeRecords conveys quite the same meaning.
Personally, I don't understand why node records are organised like that, but it seems important.
| @@ -143,4 +144,15 @@ public synchronized Optional<NodeRecord> getNode(final Bytes nodeId) { | |||
| public synchronized boolean containsNode(final Bytes nodeId) { | |||
There was a problem hiding this comment.
I don't understand why these methods are synchronized. E.g. containsNode is always just for a certain point in time. The node record could be deleted just after leaving the synchronized method. Or the node could be added just after we returned false.
Same for the other methods.
There was a problem hiding this comment.
I assume it's from an abundance of caution in a system intended to be used in a multi-threaded environment. Perhaps it would be good to overhaul it with ReentractReadWriteLock instead. It could increase performance if the discovery system is used frequently in multiple threads.
6 participants
Add some methods to aid in the development of the Samba portal client library.
#186