You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Attila Jeges (Code Review)" <ge...@cloudera.org> on 2018/03/05 16:13:53 UTC
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Attila Jeges 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 2:
(32 comments)
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: anhanced
typo: enhanced
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@11
PS2, Line 11: function
typo: functions
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@12
PS2, Line 12: Once
If
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@13
PS2, Line 13: than
typo: then
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@15
PS2, Line 15: In addition there were two functions, NewFile() and
: FileAllocateSpace() that always returned Status::OK(). I made them
: void and removed the status checks from the call sites.
Any reason you included these changes?
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@20
PS2, Line 20: funtions
typo: functions
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: make
making
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
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@245
PS2, Line 245: phase_to_fail - Gives the name
'phase_to_fail' specifies the name
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@246
PS2, Line 246: 'fdopen'
"fdopen"
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@250
PS2, Line 250: ExecuteWriteFailureTest
ExecuteWriteFailureTest()
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@376
PS2, Line 376: WriteErrorInFdopen
The name is misleading as errors in functions other than fdopen() are also tested.
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@384
PS2, Line 384: ExecuteWriteFailureTest
Even better, you should get rid off ExecuteWriteFailureTest() completely.
Move L415-L421 to the beginning of WriteErrorInFdopen and also L427-L433 to the end of WriteErrorInFdopen, also set num_of_writes to 5.
Maybe I'm missing something here. Any reason you chose to implement tests this way?
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@384
PS2, Line 384: ExecuteWriteFailureTest
Mayb it woyld be cleaner if you passed "/tmp/disk_io_mgr_test.txt" to ExecuteWriteFailureTest() as a parameter.
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@418
PS2, Line 418: tmp_file.c_str()
tmp_file would probably work too?
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@440
PS2, Line 440: Function names can be "open", "fdopen", "fseek", "fwrite"
: // and "fclose".
Why not use an enum instead?
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@445
PS2, Line 445: the given phase parameter
'phase' parameter
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@448
PS2, Line 448: string
const string& ?
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: fd < 0 || DebugFaultInjection("open")
Maybe it would be cleaner if we used inheritance and virtual functions to implement fault injection?
class DiskIoMgr {
virtual bool chk_open(int fd) { return fd >= 0; }
};
All the fault injection related members can be moved to an inherited test class:
class DiskIoMgrWithFaultInjection : public DiskIoMgr {
virtual bool chk_open(int fd) { return fd >= 0 && !DebugFaultInjection("open"); }
bool DebugFaultInjection(..);
boost::optional<pair<string, int> > faultInjectionToWrite_;
};
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@782
PS2, Line 782: (ret_status.ok() && success != 0) || DebugFaultInjection("fclose")
Shouldn't this be:
ret_status.ok() && (success != 0 || DebugFaultInjection("fclose"))
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@812
PS2, Line 812: string
const string& ?
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@812
PS2, Line 812: DebugFaultInjection
It would be safer to pass in 'errno' as a parameter. Remember that any system call inside this function may reset errno.
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@31
PS2, Line 31: Based on an errno written disk I/O related functions this class translates it
Maybe something like this instead :
This class translates 'errno' values set by disk I/O related to functions to..
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@41
PS2, Line 41: wrote
set
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@43
PS2, Line 43: relevant for the errno
corresponding to 'errno'
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@52
PS2, Line 52: gives a relevant
: /// error text for the errno.
and returns the error text corresponding to 'errno'.
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@57
PS2, Line 57: This function handles the actual reding from error text from errnoToErrorTextMap_
Maybe something like this instead:
Looks up 'errno' in 'errnoToErrorTextMap_' and sets *error_text_body to the mapped error text. Returns true if the lookup was successful and false otherwise.
Also, have you considered returning a const string* instead of passing it in as an out-parameter?
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@60
PS2, Line 60: Based on the received key value pairs as params this function creates a
: /// "key1=value1, key2=value2" string
Maybe something like this instead:
Returns a string formatted as "key1=value1, key2=value2" where key-value pairs are taken from the 'params' map.
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@25
PS2, Line 25: unordered_map<int, string> ErrnoToErrorStatusConverter::errnoToErrorTextMap_(
: {{EACCES, "Access denied for the process' user"},
: {EINTR, "Internal error occured."},
: {EINVAL, "Invalid inputs."},
: {EMFILE, "Process level opened file descriptor count is reached."},
: {ENAMETOOLONG,
: "Either the path length or a path component exceeds the maximum length."},
: {ENFILE, "OS level opened file descriptor count is reached."},
: {ENOENT, "The given path doesn't exist."},
: {ENOSPC, "No space left on device."},
: {ENOTDIR, "It is not a directory."},
: {EOVERFLOW, "File size can't be represented."},
: {EROFS, "The file system is read only."},
: {EAGAIN, "Resource temporarily unavailable."},
: {EBADF, "The given file descriptor is invalid."},
: {ENOMEM, "Not enough memory."},
: {EFBIG, "Maximum file size reached."},
: {EIO, "Disk level I/O error occured."},
: {ENXIO, "Device doesn't exist."}});
Where did you get the content of the map from?
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.cc@57
PS2, Line 57: errno
I think, it would be safer to pass in 'errno' as a parameter to GetErrorText() and all the other relevant functions in this class. Remember that any system call may reset 'errno' if it fails.
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.cc@66
PS2, Line 66: auto&
const auto& ?
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.cc@74
PS2, Line 74: GetErrorTextBody(string* error_text_body)
Consider returning const string *instead (nullptr if the lookup fails).
--
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: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 16:13:53 +0000
Gerrit-HasComments: Yes