Todd Lipcon has posted comments on this change.

Change subject: WIP: Integration test for replay cache
......................................................................


Patch Set 5:

Been looking at the new test and the code for the last couple hours.

A couple concerns I have from running the test:
- at least on my system, it doesn't trigger any leader elections. Perhaps we 
need a thread which is randomly cycling through the tservers and stopping them? 
Or explicitly call StartElection to force term bumps and re-elections?
- I ran it through coverage, and perhaps due to the above, there are some lines 
in ResultTracker that aren't covered. For example, in 
RecordCompletionAndRespond:

    24820:  225:    if (MustHandleRpc(handler_attempt_no, completion_record, 
ongoing_rpc)) {
    12410:  226:      if (ongoing_rpc.context != nullptr) {
    11873:  227:        if (PREDICT_FALSE(ongoing_rpc.response != response)) {
     3746:  228:          
ongoing_rpc.response->CopyFrom(*completion_record->response);
     1873:  229:        }
    11873:  230:        LogAndTraceAndRespondSuccess(ongoing_rpc.context, 
*ongoing_rpc.response);
    11873:  231:      }
    24820:  232:      orpc_iter = 
completion_record->ongoing_rpcs.erase(orpc_iter);
    12410:  233:    } else {
    #####:  234:      ++orpc_iter;
        -:  235:    }
        -:  236:  }
    30000:  237:}


(#### indicates a non-covered line). Not sure yet whether this line is actually 
impossible to hit, or just not hit due to a test deficiency.

Will keep reading through this trying to fully understand what's going on. To 
aid that, one suggestion is to do another pass over the ResultTracker API and 
explain some more of the invariants. For example: if one thread uses 
ChangeDriver, what requirements does that impose on the previous driver? (I 
think it means that the previous driver must not call RecordSuccess, and it's 
up to the caller to ensure that the previous driver can't succeed if a new 
driver steals it?)

-- 
To view, visit http://gerrit.cloudera.org:8080/3519
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: No

Reply via email to