Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12308 )

Change subject: KUDU-2665: LBM may delete containers with live blocks
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/12308/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12308/2//COMMIT_MSG@12
PS2, Line 12: That's because
> Our jenkins job are using dist-test to have efficient test runs (tests are
FWIW, I also wasn't able to repro this bug using dist-test's loop feature. I 
was only able to repro from time to time on an el6.6 box in RELEASE mode.

I'm not sure how easy it is to submit jobs to dist-test outside of Cloudera's 
infrastructure.


http://gerrit.cloudera.org:8080/#/c/12308/3/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:

http://gerrit.cloudera.org:8080/#/c/12308/3/src/kudu/fs/log_block_manager-test.cc@1878
PS3, Line 1878:     // The size of the container will be 10*4KB after alignment.
This may not necessarily be true; the filesystem block size may vary from 
filesystem to filesystem. For example, on my el6.6 testing box, the block size 
is 2K.

That said, it doesn't seem like it matters. It seems like it's enough to write 
one block, then write a block sized to FLAGS_log_container_max_size (i.e. the 
"special" block), then delete the first block.


http://gerrit.cloudera.org:8080/#/c/12308/3/src/kudu/fs/log_block_manager-test.cc@1916
PS3, Line 1916:     if (close_block) {
The idea here is to test Abort(), right? If so, could you note that in the "// 
Close and reset the writer" comment?


http://gerrit.cloudera.org:8080/#/c/12308/3/src/kudu/fs/log_block_manager-test.cc@1928
PS3, Line 1928:       ASSERT_EQ(Status::NotFound("").CodeAsString(), 
s.CodeAsString());
Simpler to do:

  ASSERT_TRUE(s.IsNotFound());

Or even combine with L1927:

  ASSERT_TRUE(bm_->OpenBlock(block_id, &block).IsNotFound());



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I894f32b1c164ae7770c92171850edd167dfaf8ad
Gerrit-Change-Number: 12308
Gerrit-PatchSet: 3
Gerrit-Owner: helifu <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Hao Hao <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <[email protected]>
Gerrit-Comment-Date: Fri, 01 Feb 2019 20:54:53 +0000
Gerrit-HasComments: Yes

Reply via email to