Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/9420 )
Change subject: IMPALA-3866: Improve error reporting for scratch write errors ...................................................................... Patch Set 8: (16 comments) Thanks for taking a look, Tim! http://gerrit.cloudera.org:8080/#/c/9420/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9420/7//COMMIT_MSG@7 PS7, Line 7: IMPALA-3866: Improve error reporting for scratch write errors > nit: colon after JIRA number Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc File be/src/runtime/io/disk-io-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@347 PS7, Line 347: void DiskIoMgrTest::ExecuteWriteFailureTest(DiskIoMgr* io_mgr, const string& file_name, > nit: extra blank line. Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@355 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 Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@362 PS7, Line 362: io_mgr->SetLocalFileSystem(fs); > Same with new_range - can't we stack allocate it? Removed this. http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@380 PS7, Line 380: nullptr, nullptr, nullptr, data, > I don't think allocated the pointer on the heap is needed, is it? Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@385 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. http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-with-fault-injection.h File be/src/runtime/io/disk-io-mgr-with-fault-injection.h: http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@46 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. http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@48 PS7, Line 48: > Let's add an "override" specifier to the end of overriding functions. Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@58 PS7, Line 58: > Our convention is to not use underscores on struct members, since they're m Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.h File be/src/runtime/io/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.h@401 PS7, Line 401: REMOTE_S3_DISK_OFFSET, > I feel like maybe there's already too much functionality in the DiskIoMgr c Good idea, thx! Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.h@404 PS7, Line 404: }; > I also think if we're going to pull these out into functions, we should jus Makes sense! Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.cc@1120 PS7, Line 1120: > We can unnest the body of this "else" now that we're doing an early return Thx! Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/errno-to-error-status-converter.h File be/src/runtime/io/errno-to-error-status-converter.h: http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/errno-to-error-status-converter.h@34 PS7, Line 34: > Maybe we can just name this ErrorConverter to be more concise? Sure, Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/errno-to-error-status-converter.h@43 PS7, Line 43: > Can we document what 'params' does? Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/errno-to-error-status-converter.cc File be/src/runtime/io/errno-to-error-status-converter.cc: http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/errno-to-error-status-converter.cc@23 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! Done (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?) http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/errno-to-error-status-converter.cc@68 PS7, Line 68: > nit: needs space Done -- To view, visit http://gerrit.cloudera.org:8080/9420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings 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 <gaborkas...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 12 Mar 2018 13:56:47 +0000 Gerrit-HasComments: Yes
