Implementation of multiRead

[john.ousterhout, 2011-05-22 22:33:16]

http://rccodereview.appspot.com/35001/diff/1/5
File src/MasterServer.cc (right):

http://rccodereview.appspot.com/35001/diff/1/5#newcode319
src/MasterServer.cc:319: *status = STATUS_TABLE_DOESNT_EXIST;
It doesn't appear that this path is exercised by the tests; can you add a test
for this condition to make sure that errors are propagated quickly?

http://rccodereview.appspot.com/35001/diff/1/5#newcode324
src/MasterServer.cc:324: *status = STATUS_OBJECT_DOESNT_EXIST;
Same comment here.

http://rccodereview.appspot.com/35001/diff/1/7
File src/MasterTest.cc (right):

http://rccodereview.appspot.com/35001/diff/1/7#newcode224
src/MasterTest.cc:224: void test_multiRead_oneT_oneO() {
These tests look good, but you could probably get by without all three. The idea
of unit tests is to exercise all of the major control paths and decisions in the
code. However, the case of objects in different tables isn't reflected in the
code: it's handled just the same as objects in the same table, right?  Thus, you
probably don't need a special case for that condition.

http://rccodereview.appspot.com/35001/diff/1/12
File src/RamCloudTest.cc (right):

http://rccodereview.appspot.com/35001/diff/1/12#newcode27
src/RamCloudTest.cc:27: namespace RAMCloud {
I'm a little confused about this file: from the name, I would guess that it
provides tests for RamCloud.cc, but it appears to be testing mostly other files.
The code in RamCloud.cc looks pretty simple, so you could probably get by with a
single test for that.

It looks like some of what you're testing here may be code in ObjectFinder.cc?
However, tests for that code should go in ObjectFinderTest.cc; I don't see
ObjectFinderTest.cc in your list of modified files, but there should be new
tests in that file that verify your changes.

http://rccodereview.appspot.com/35001/diff/1/12#newcode177
src/RamCloudTest.cc:177: void test_multiRead_badTable() {
This test and the following one appear to be testing error conditions, which
only manifest themselves in the multiRead method of MasterServer.cc; thus I'd
move them to the tests for that file (MasterTest.cc?).

[Ankita, 2011-05-23 21:32:26]

http://rccodereview.appspot.com/35001/diff/1/3
File src/MasterClient.cc (right):

http://rccodereview.appspot.com/35001/diff/1/3#newcode321
src/MasterClient.cc:321: for (uint32_t i = 0; i < requests.size(); i++){
foreach

http://rccodereview.appspot.com/35001/diff/1/3#newcode323
src/MasterClient.cc:323: new(&req, APPEND) MultiReadRequestPart;
use as a constructor (default)

http://rccodereview.appspot.com/35001/diff/1/4
File src/MasterClient.h (right):

http://rccodereview.appspot.com/35001/diff/1/4#newcode34
src/MasterClient.h:34: 
meta comments style

http://rccodereview.appspot.com/35001/diff/1/4#newcode57
src/MasterClient.h:57: uint64_t* version;
not a pointer

http://rccodereview.appspot.com/35001/diff/1/4#newcode62
src/MasterClient.h:62: Status* status;
not a pointer

http://rccodereview.appspot.com/35001/diff/1/4#newcode72
src/MasterClient.h:72: };
semicolons. here and later.

http://rccodereview.appspot.com/35001/diff/1/5
File src/MasterServer.cc (right):

http://rccodereview.appspot.com/35001/diff/1/5#newcode282
src/MasterServer.cc:282: *      response buffer for this type of request. It
contains
pre allocated space only for header. we add other space.

http://rccodereview.appspot.com/35001/diff/1/5#newcode311
src/MasterServer.cc:311: *status = STATUS_OK;
pull up as a part of constructor

http://rccodereview.appspot.com/35001/diff/1/5#newcode319
src/MasterServer.cc:319: *status = STATUS_TABLE_DOESNT_EXIST;
continue

http://rccodereview.appspot.com/35001/diff/1/5#newcode324
src/MasterServer.cc:324: *status = STATUS_OBJECT_DOESNT_EXIST;
continue

http://rccodereview.appspot.com/35001/diff/1/5#newcode327
src/MasterServer.cc:327: if (*status == STATUS_OK){
no need of if

http://rccodereview.appspot.com/35001/diff/1/5#newcode336
src/MasterServer.cc:336: handle->length());
try with just append (add both sizes)

http://rccodereview.appspot.com/35001/diff/1/5#newcode338
src/MasterServer.cc:338: respHdr.count++;
assign in the beginning

http://rccodereview.appspot.com/35001/diff/1/8
File src/ObjectFinder.cc (right):

http://rccodereview.appspot.com/35001/diff/1/8#newcode90
src/ObjectFinder.cc:90: *      The coordinator has no record of a table.
change!!!!

http://rccodereview.appspot.com/35001/diff/1/8#newcode94
src/ObjectFinder.cc:94: *      and the SessionRef corresponding to that master.
remove 93 and 94.

http://rccodereview.appspot.com/35001/diff/1/8#newcode123
src/ObjectFinder.cc:123: requestBins.push_back(currentRequestBin);
move instead of copy. can pull this up, then get ref to the last object and then
put values in there.

http://rccodereview.appspot.com/35001/diff/1/9
File src/ObjectFinder.h (right):

http://rccodereview.appspot.com/35001/diff/1/9#newcode41
src/ObjectFinder.h:41: Transport::SessionRef sessionref;
sessionRef

http://rccodereview.appspot.com/35001/diff/1/9#newcode42
src/ObjectFinder.h:42: std::vector<MasterClient::ReadObject> requests;
readobject pointers

http://rccodereview.appspot.com/35001/diff/1/10
File src/RamCloud.cc (right):

http://rccodereview.appspot.com/35001/diff/1/10#newcode91
src/RamCloud.cc:91: {
masterrequests comment - remove

http://rccodereview.appspot.com/35001/diff/1/10#newcode102
src/RamCloud.cc:102: for (uint32_t j = 0; j < numBins; j++){
foreach

http://rccodereview.appspot.com/35001/diff/1/12
File src/RamCloudTest.cc (right):

http://rccodereview.appspot.com/35001/diff/1/12#newcode126
src/RamCloudTest.cc:126: 
status initializations not to 0.

http://rccodereview.appspot.com/35001/diff/1/13
File src/Rpc.h (right):

http://rccodereview.appspot.com/35001/diff/1/13#newcode190
src/Rpc.h:190: */
put in the requestpart as part here

http://rccodereview.appspot.com/35001/diff/1/13#newcode197
src/Rpc.h:197: * Included here to fulfill requirements in common code.
always be status ok. reword.