David Ribeiro Alves has posted comments on this change.
Change subject: Add a ResultTracker class that will track server side results
Patch Set 3:
Line 40: lock_guard<simple_spinlock> l(&lock_);
> I suspect it will be, as nearly every RPC will call CheckDuplicate(), no?
yeah, definitely something we can optimize for later, if this comes up in the
hotspots list. we usually follow the policy of implementing a simple solution
first and optimizing it later if needed.
Added a TODO in this regard though.
Line 78: LOG(FATAL) << "Wrong state";
> OK, could we at least log what the wrong state was?
true, we can. Done.
Line 119: ClientState* client_state = FindOrDie(clients_, client_id);
: CompletionRecord* completion_record =
> Well, this goes back to the lack of documentation thing. :)
Well this is not a "new" module per se, IMO this is an change in the semantics
of the rpc module.
A couple of things:
1 - This is not meant to be used by itself in the majority of cases (single
server)as this is hidden behind RpcContext. In the few cases where it's meant
to be used stand alone (Write() for one) yes, care would have to be taken but
if this fails what would it even reply to the client "Request lost?". Which
takes us to point 2.
2 - This doesn't always reply to the client. For instance in for writes If a
follower is calling this on some op that came from the leader there might not
(and likely won't) be a client to reply to, we're just storing the response in
the case it will eventually appears. Meaning this would fail silently if we
didn't crash, meaning we'd have a bug we don't know exists as a new client rpc
would be treated as new (an instance of the data corruption we have now).
I've improved the docs to reflect that this requires that CheckDuplicate() was
called before, please take a look, but I don't want to go into too much detail
on every possible usage of this.
To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: David Ribeiro Alves <david.al...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <david.al...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>