Gabor Kaszab has posted comments on this change. ( )

Change subject: IMPALA-3866: Improve error reporting for scratch write errors

Patch Set 8:


Thanks for taking a look, Tim!
Commit Message:
PS7, Line 7: IMPALA-3866: Improve error reporting for scratch write errors
> nit: colon after JIRA number
File be/src/runtime/io/
PS7, Line 347: void DiskIoMgrTest::ExecuteWriteFailureTest(DiskIoMgr* io_mgr, 
const string& file_name,
> nit: extra blank line.
PS7, Line 355:     LOG(ERROR) << "Error creating temp file " << file_name << " 
of size 100";
> Can't we stack allocate this? The memory shouldn't be referenced once write
PS7, Line 362:   io_mgr->SetLocalFileSystem(fs);
> Same with new_range - can't we stack allocate it?
Removed this.
PS7, Line 380:           nullptr, nullptr, nullptr, data,
> I don't think allocated the pointer on the heap is needed, is it?
PS7, Line 385: }
> We already overwrote the value of 'write_range' on l380, so this doesn't ac
That's right, removed the write_range param. Also I noticed that it's not 
necessary in this case to pass a write_range to WriteValidateCallback() as it 
only uses it if the write succeeds.
File be/src/runtime/io/disk-io-mgr-with-fault-injection.h:
PS7, Line 46:
> Let's document explicitly which functions they're wrapping.
I docemented the name of the wrapped functions in disk-io-mgr.h. These are 
overriding the wrapper function from that class.
PS7, Line 48:
> Let's add an "override" specifier to the end of overriding functions.
PS7, Line 58:
> Our convention is to not use underscores on struct members, since they're m
File be/src/runtime/io/disk-io-mgr.h:
PS7, Line 401:     REMOTE_S3_DISK_OFFSET,
> I feel like maybe there's already too much functionality in the DiskIoMgr c
Good idea, thx!
PS7, Line 404:   };
> I also think if we're going to pull these out into functions, we should jus
Makes sense!
File be/src/runtime/io/
PS7, Line 1120:
> We can unnest the body of this "else" now that we're doing an early return
File be/src/runtime/io/errno-to-error-status-converter.h:
PS7, Line 34:
> Maybe we can just name this ErrorConverter to be more concise?
Sure, Done
PS7, Line 43:
> Can we document what 'params' does?
File be/src/runtime/io/
PS7, Line 23:
> We can just do #include "common/names.h" under the other includes and remov
I wasn't aware of this common/names.h, thx!

(Apparently this doesn't work for std::unordered_map, however would work for 
boost::unordered_map. Do you know if there is a preference between them?)
PS7, Line 68:
> nit: needs space

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab <>
Gerrit-Reviewer: Attila Jeges <>
Gerrit-Reviewer: Gabor Kaszab <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Mon, 12 Mar 2018 13:56:47 +0000
Gerrit-HasComments: Yes

Reply via email to