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 4: (31 comments) Thanks for taking a look, Attila and Tim! http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@9 PS2, Line 9: enhanced > typo: enhanced Done http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@11 PS2, Line 11: function > typo: functions Done http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@12 PS2, Line 12: If a > If Done http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@13 PS2, Line 13: then > typo: then Done http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@20 PS2, Line 20: function > typo: functions Done http://gerrit.cloudera.org:8080/#/c/9420/2/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/2/be/src/runtime/io/disk-io-mgr-test.cc@242 PS2, Line 242: > Probably you should move L242-L255 after L236 Rebase solved this :) http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@242 PS2, Line 242: _1) > making Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@245 PS2, Line 245: (*new_range)->SetData(reint > 'phase_to_fail' specifies the name Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@246 PS2, Line 246: PECT_OK( > "fdopen" Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@250 PS2, Line 250: > ExecuteWriteFailureTest() Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@376 PS2, Line 376: > The name is misleading as errors in functions other than fdopen() are also Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@384 PS2, Line 384: write_range, nu > Mayb it woyld be cleaner if you passed "/tmp/disk_io_mgr_test.txt" to Execu Made "/tmp/disk_io_mgr_test.txt" a parameter, also removed it being hard-coded from the error text. http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@418 PS2, Line 418: > tmp_file would probably work too? Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h File be/src/runtime/io/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@443 PS2, Line 443: > nit: fault_injection_to_write_ Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@443 PS2, Line 443: > It might be more readable if we define a struct here. Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@445 PS2, Line 445: disk_thread_group_; > 'phase' parameter Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@448 PS2, Line 448: ached_ > const string& ? Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@765 PS2, Line 765: disk_id = disk_queue->disk_id; > Seems a good idea not to mess the prod code for fault injection. I'll give Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@782 PS2, Line 782: // Get the next reader and remove the reader so that another disk > Shouldn't this be: Indeed, thx. Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@812 PS2, Line 812: > const string& ? Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@812 PS2, Line 812: > It would be safer to pass in 'errno' as a parameter. Remember that any syst This is the point where I set errno to some desired value. Once it is set, not much happening until GetErrorStatusFromErrno() is called. I modified that function to receive errno as param. Is this fine or should I think of any other issues? http://gerrit.cloudera.org:8080/#/c/9420/2/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/2/be/src/runtime/io/errno-to-error-status-converter.h@28 PS2, Line 28: /// This class translates 'errno' values set by disk I/O related functions to Status > Avoid "using" declarations in headers. Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@31 PS2, Line 31: > Maybe something like this instead : Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@41 PS2, Line 41: TextM > set Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@43 PS2, Line 43: e, const Params& param > corresponding to 'errno' Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@52 PS2, Line 52: : > and returns the error text corresponding to 'errno'. Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@57 PS2, Line 57: > Maybe something like this instead: Thanks for the suggestion! Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@60 PS2, Line 60: tic std::string GetParamsString(const Params& params); : }; > Maybe something like this instead: Thanks! Done http://gerrit.cloudera.org:8080/#/c/9420/2/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/2/be/src/runtime/io/errno-to-error-status-converter.cc@57 PS2, Line 57: > I think, it would be safer to pass in 'errno' as a parameter to GetErrorTex Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.cc@66 PS2, Line 66: ng re > const auto& ? Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.cc@74 PS2, Line 74: > Consider returning const string *instead (nullptr if the lookup fails). 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: 4 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: Tue, 06 Mar 2018 15:10:57 +0000 Gerrit-HasComments: Yes
