Adar Dembo has posted comments on this change.

Change subject: KUDU-1508: script for testing presence of bug and finding upper 

Patch Set 1:


> can you write in the commit message or in the test header something
 > about what the finding here means? if I'm interpreting it
 > correctly, we're trying to find the max number of extents in the
 > tree before it becomes multi-level, and that's dependent on the
 > file system's block size?

Yes, I'll note that in the commit description.
File src/kudu/experiments/KUDU-1508/hole_punch_range.c:

Line 32:     fprintf(stderr, "usage: %s <path> <start block> <end block> 
<stride>\n", argv[0]);
> clarify whether the file should exist or not, and how large it should be?
Clarified the existence and the arguments.

Size doesn't matter; fallocate() will no-op if you punch a hole beyond the size 
of the file.

Line 40:   int fd = open(argv[1], O_EXCL | O_WRONLY, 0644);
> O_EXCL without O_CREAT is undefined
Whoops, dropped O_EXCL.

Line 54:   for (block_num = start_block; block_num < end_block; block_num += 
stride) {
> any reason not to use C99 here?
Not a huge deal, but I didn't want to have to pass -std=c99 with older 
compilers (i.e. gcc 4.4 on el6). Given that this is the only C99ism, it seemed 
like a reasonable trade-off.
File src/kudu/experiments/KUDU-1508/

PS1, Line 70: 1
> instead of '1' how about using a string like 'fail_ok' so it's clearer
Sure, will use "fail_ok".

I couldn't get "... || :" to work with the run() method, though maybe there's 
some quoting that would make it work.

PS1, Line 77: -l 
> can you use the longer form (--length) here and elsewhere for flags that ar
Sure, though I think this is the only place I could use a longopt.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I710918a153a9e8e05e989fe63281891c9ebc7178
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to