Peer reservations #2935
Peer reservations #2935
Conversation
thejohnfreeman
requested review from
scottschurr,
seelabs,
miguelportilla and
HowardHinnant
Jenkins Build SummaryBuilt from this commit Built at 20190805 - 23:42:42 Test Results
|
:-( |
|
@MarkusTeufelberger, obviously having the conversation on our internal wiki isn't ideal (we're moving away from it ourselves, in favor of open conversation on GitHub). Here's the relevant content in the form of an issue: #2938 |
|
Thanks! :-) Much clearer what this is supposed to do (without having to guess at the intentions). |
There was a problem hiding this comment.
I should start out by saying I have a great deal of sympathy for the position you're in. When I started working on the rippled code base a lot of the conventions did not make sense to me. Many of them still don't "make sense", but I've learned to live with most of them. A few of them I've managed to adjust so they make more sense to me, but possible less sense to others.
Regarding adding src/ripple/basics/Result.h, I believe we already have a (less elegant) approach to solving that problem, at least at the RPC level where you are using it at the moment. Take a look at https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/ErrorCodes.h. That file provides a way to inject an error message into a Json::Value (make_error) and test a Json::Value for the presence of a such an error (contains_error). I suggest you see if you can coerce those facilities to match your needs.
There are definitely problems with error handling in the rippled code base. I'm not trying to defend the current state of affairs. But I think it's a better idea to stick with the approaches we have until someone can come up with a proposal that we think significantly improves the situation. Otherwise we're simply proliferating error handling machinery, which in the end makes the matter worse/more confusing.
|
@thejohnfreeman For unity builds, |
| // TODO: Make a formal team decision that we do not want to raise an | ||
| // error upon a collision. | ||
| auto emplaced = context.app.peerReservations().emplace( | ||
| std::make_pair(identity, overlay::PeerReservation{identity, name})); |
There was a problem hiding this comment.
I realize its a WIP but you might get away with simply storing an optional string versus a PeerReservation.
|
@thejohnfreeman You might want to consider having add and remove accept an array of objects allowing the caller to specify one or more pubkey/name pairs in a single call. |
Codecov Report
@@ Coverage Diff @@
## develop #2935 +/- ##
==========================================
- Coverage 69.38% 69.3% -0.08%
==========================================
Files 708 710 +2
Lines 56426 56490 +64
==========================================
+ Hits 39152 39153 +1
- Misses 17274 17337 +63
Continue to review full report at Codecov.
|
|
Sadly, this branch is is not currently building for me on macOS using Boost 1.70.0. It looks like 1.70.0 lost the It's possible that a rebase atop 1.3.0-b6 will fix the issues. It looks like the branch is currently sitting atop 1.3.0-b3 |
thejohnfreeman
force-pushed
the
1712-peer-reservations
branch
from
3f5098e
to
1c01434
Compare
There was a problem hiding this comment.
In general the code looks very nice. I like the way you've made an effort to work in coordination with local conventions, even though you may find those conventions irksome. That's not to say we can never change these conventions. But if we do, the team should agree that the change should be made and we should then fix it everywhere (in my not so humble opinion). So you're doing something that some other folks in this code base have not been so good at. Thank you.
I did leave some comments that I'd either like to see addressed or understand a motivation for not making the change. I'm good either way, but I think some of the points are worth discussing.
Thanks. Good work.
| @@ -231,7 +231,7 @@ class Counts | |||
| map ["in"] << m_in_active << "/" << m_in_max; | |||
| map ["out"] << m_out_active << "/" << m_out_max; | |||
| map ["fixed"] = m_fixed_active; | |||
| map ["cluster"] = m_cluster; | |||
| map ["reserved"] = m_reserved; | |||
There was a problem hiding this comment.
I don't think this is written to a public API, so I'm guessing the rename from cluster to reserved won't hurt anybody. But that would be a good thing to research and confirm. And maybe leave a comment once you've done the research. Thanks.
| // REVIEWER: What is the policy on forward declarations? We can use one | ||
| // for DatabaseCon. | ||
| DatabaseCon* connection_; | ||
| table_type table_; |
There was a problem hiding this comment.
My spidey sense may be telling me about a race condition here? What assurances do we have that, say, upsert() and begin() cannot be called on different threads? Now or in the future?
If you can't give that assurance, and document why that assurance is valid, then I think accesses to table_ will need to be guarded with an mutex. That would also mean removing the iterator interface that returns your internal implementation. If this is necessary then I'd suggest replacing the iterator interface with a call that returns a sorted std::vector<PerReservation>.
| if (slot.cluster()) | ||
| item ["cluster"] = "yes"; | ||
| if (slot.reserved()) | ||
| item ["reserved"] = "yes"; |
There was a problem hiding this comment.
Similar to Counts.h, I don't think this is written to a public API, so I'm guessing the rename from cluster to reserved won't hurt anybody. But that would be a good thing to research and confirm. And maybe leave a comment once you've done the research. Thanks.
thejohnfreeman
force-pushed
the
1712-peer-reservations
branch
from
530f3bb
to
423baab
Compare
| // https://stackoverflow.com/q/49651835/618906 | ||
| // Regardless, we don't expect this function to be called often, or | ||
| // for the table to be very large, so this less-than-ideal | ||
| // remove-then-insert is acceptable in order to present a better API. |
There was a problem hiding this comment.
FWIW, I suspect the awkwardness of this method suggests that the right choice for table_'s container would have been a map or unordered_map. Then you'd be able to replace the value without needing to remove the entire node. But I also know this has been discussed plenty, and I'm not asking for any changes.
There was a problem hiding this comment.
I disagree. I think it's a problem with unordered_set. If it had an insert_or_assign method the same as unordered_map, then there would be no awkwardness. There is no technical reason it cannot have such a method. I don't think a deficiency in one standard container means the right choice is to pick a differently deficient container.
There was a problem hiding this comment.
From the perspective of the standard designers there is no deficiency. The key to both a set and a map must be immutable. If you were allowed to mutate a key in place that would corrupt the data structure. So, sensibly enough, the interface doesn't allow you to mutate the key (without working hard).
A set only has a key, and there are no options for a partially non-const key. If you need a partially non-const key, well, that's what map is for.
There was a problem hiding this comment.
Can you define "mutate the key"? I don't see insert_or_assign as a mutation of the key. I see it as an atomic replacement. It is not like asking the container to return an iterator to a non-const key where I can mutate the key outside the purview of the container. I am asking the container itself to atomically remove one key and insert another. It has full control of its invariants before, during, and after that process. No corruption would be possible. I don't need the key_type or value_type of unordered_set to be mutable at all. I just need an amotic replace/upsert/insert_or_assign/whatever-you-want-to-call-it method.
There was a problem hiding this comment.
The reason I was referring to "mutate the key" is because the part of the key that determines the location in the set does not change during the "atomic replacement". The only thing that changes is the description_. So you could leave the key in place and "mutate the key" by changing (only) description_.
I'm not going to argue with you about whether insert_or_assign should be a part of the standard set interface.
I want to get this PR started now, while the work is in progress, to get more eyes and some direction. I'm going to link some internal resources for now; once they are polished, I'd like to find some way to publish them publicly.
For context, see #2938 (and JIRA 1712 if you have access).
I've written what I've deduced to be the steps for adding a command to rippled on our internal wiki. I've been following those steps below. Please comment there or here with any corrections or blind spots (especially blind spots; I don't know what I don't know).
I've left some questions in comments in the changes below. Please take a look and see if you can answer any. If you just want to skim for them, Ctrl-f for
?.I'm having trouble figuring out how I should think about and handle "public key for a node" as a string. If there are any docs on this, please point me to them.
Similarly, our RPC "framework" seems to be immune to function abstraction. I've taken a stab at it below, but I'm not happy with it. Here's the pattern I see in the rest of the code:
The problem is the temporary variable combined with the early return for an error. If I try to put this in a function, I can't return both, so I end up taking an out parameter for the value. But I don't think I want to test for an empty
Json::Valuein the event of success, so I take that as an out parameter too and return abool. It's all very gross to me. This kind of control flow is better suited by exceptions, in my opinion, or by an Error monad (do we have one available?). Thoughts?