You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Gabor Kaszab (Code Review)" <ge...@cloudera.org> on 2018/02/23 17:05:03 UTC
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Gabor Kaszab has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9420
Change subject: IMPALA-3866 Improve error reporting for scratch write errors
......................................................................
IMPALA-3866 Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are anhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level function and an
error message for displaying purposes. Once any of these functions
fail than the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned funtions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/errno-to-error-status-converter.cc
A be/src/runtime/io/errno-to-error-status-converter.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
10 files changed, 320 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/1
--
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: newchange
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
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
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 7:
(16 comments)
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
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:
nit: extra blank line.
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@355
PS7, Line 355: int32_t* data = pool_.Add(new int32_t);
Can't we stack allocate this? The memory shouldn't be referenced once writer is unregistered. I.e.
int32_t data = rand();
AddWriteRange(..., &data, ...);
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@362
PS7, Line 362: WriteRange** new_range = pool_.Add(new WriteRange*);
Same with new_range - can't we stack allocate it?
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@380
PS7, Line 380: write_range = pool_.Add(new WriteRange*);
I don't think allocated the pointer on the heap is needed, is it?
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@385
PS7, Line 385: *write_range = pool_.Add(new WriteRange(file_name, offset, 0, callback));
We already overwrote the value of 'write_range' on l380, so this doesn't actually return anything to the caller.
Maybe we don't need 'write_range' to be an argument, even?
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: virtual int Open(const char* file, int option1, int option2);
Let's document explicitly which functions they're wrapping.
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.
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@58
PS7, Line 58: function_
Our convention is to not use underscores on struct members, since they're meant to be public.
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: // Wrapper functions around open(), fdopen(), fseek(), fwrite() and fclose(). The
I feel like maybe there's already too much functionality in the DiskIoMgr class. Can we pull out these functions into a helper class that we can subclass? Maybe something like LocalFilesystem?
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.h@404
PS7, Line 404: virtual int Open(const char* file, int option1, int option2);
I also think if we're going to pull these out into functions, we should just make them return Status so they're consistent with the usual Impala calling conventions. This would also slightly reduce the amount of logic in the core DiskIoMgr class, which would be a win.
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: } else {
We can unnest the body of this "else" now that we're doing an early return on the other branch.
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: ErrnoToErrorStatusConverter
Maybe we can just name this ErrorConverter to be more concise?
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/errno-to-error-status-converter.h@43
PS7, Line 43: params
Can we document what 'params' does?
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: using std::unordered_map;
We can just do #include "common/names.h" under the other includes and remove these using statements.
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/errno-to-error-status-converter.cc@68
PS7, Line 68: for(const auto& item : params) {
nit: needs space
--
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: 7
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Mar 2018 00:53:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Attila Jeges, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9420
to look at the new patch set (#5).
Change subject: IMPALA-3866 Improve error reporting for scratch write errors
......................................................................
IMPALA-3866 Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
A be/src/runtime/io/disk-io-mgr-with-fault-injection.cc
A be/src/runtime/io/disk-io-mgr-with-fault-injection.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/errno-to-error-status-converter.cc
A be/src/runtime/io/errno-to-error-status-converter.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
12 files changed, 458 insertions(+), 76 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/5
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Mar 2018 13:56:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 6:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc
File be/src/runtime/io/disk-io-mgr-with-fault-injection.cc:
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@38
PS6, Line 38: {
> nit: Parentheses not necessary (here and below).
Done
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@52
PS6, Line 52: int64_t
> fwrite() retuns size_t
Done
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@53
PS6, Line 53: return -1;
> fwrite() should return 0 in case of failure.
Done
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@58
PS6, Line 58: return -1;
> fclose() should return EOF in case of failure.
Done
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc@1099
PS6, Line 1099: WriteRange*
> const WriteRange* ?
Done
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc@1099
PS6, Line 1099: int64_t
> fwrite() returns size_t
Done
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc@1106
PS6, Line 1106: file_handle
> file_handle != nullptr
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: 6
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Mar 2018 15:10:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Attila Jeges, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9420
to look at the new patch set (#4).
Change subject: IMPALA-3866 Improve error reporting for scratch write errors
......................................................................
IMPALA-3866 Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
A be/src/runtime/io/disk-io-mgr-with-fault-injection.cc
A be/src/runtime/io/disk-io-mgr-with-fault-injection.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/errno-to-error-status-converter.cc
A be/src/runtime/io/errno-to-error-status-converter.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
12 files changed, 449 insertions(+), 76 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/4
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 15:
Hey Tim, Dan,
I think I addressed all your comments. Could you please take a look and submit if you find everything fine?
--
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: 15
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 16:05:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
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 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9420/5/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/5/be/src/runtime/io/disk-io-mgr-test.cc@191
PS5, Line 191: const string& phase_to_fail, int error_code
nit: It would be nice to make formal parameter names consistent across function calls. SetWriteFaultInjection() uses 'function_name' and 'err_no'.
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-test.cc@350
PS5, Line 350: const string& phase_to_fail, int error_code
same as above
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-test.cc@378
PS5, Line 378: tmp_file
nit: 'file_name' to be consistent with the formal parameter name in ExecuteWriteFailureTest().
http://gerrit.cloudera.org:8080/#/c/9420/5/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/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@56
PS5, Line 56: errno_
nit: Maybe this should be called 'err_no_' for consistency?
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc
File be/src/runtime/io/disk-io-mgr-with-fault-injection.cc:
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@38
PS5, Line 38: CheckOpen
Another approach would be to abstract away the open() syscall itself, instead of te error-checking logic.
We should have an Open() function instead of CheckOpen():
int DiskIoMgr::Open(..) {
// Just call open() syscall
return open(..);
}
int DiskIoMgrWithFaultInjection::Open(..) {
if (DebugFaultInjection("open")) {
// Return -1 to simulate failure.
// (errno has been set by DebugFaultInjection())
return -1;
}
return DiskIoMgr::Open(..);
}
The advantage of this approach would be that if fault injection is used, we don't even call open(), we just fail with the preset errno.
What do you think?
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@38
PS5, Line 38: DiskIoMgr::CheckOpen(file_desc) && !DebugFaultInjection("open");
Not sure about this, but maybe the order of operands should be reversed (here and below) to guarantee that open() will fail with the preset errno if fault injection is used.
--
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: 5
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Mar 2018 14:38:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Attila Jeges, Tim Armstrong, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9420
to look at the new patch set (#15).
Change subject: IMPALA-3866: Improve error reporting for scratch write errors
......................................................................
IMPALA-3866: Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/error-converter.cc
A be/src/runtime/io/error-converter.h
A be/src/runtime/io/local-file-system-with-fault-injection.cc
A be/src/runtime/io/local-file-system-with-fault-injection.h
A be/src/runtime/io/local-file-system.cc
A be/src/runtime/io/local-file-system.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/error-util.cc
M be/src/util/error-util.h
17 files changed, 624 insertions(+), 88 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/15
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 15
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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:
(5 comments)
I took a quick pass to understand it at a high level. Left a few comments but I think the approach makes sense. I'll let Attila finish the review before taking another look.
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG
Commit Message:
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?
Seems like good cleanup to me.
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: faultInjectionToWrite_
nit: fault_injection_to_write_
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@443
PS2, Line 443: pair<string, int>
It might be more readable if we define a struct here.
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: using std::unordered_map;
Avoid "using" declarations in headers.
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/tmp-file-mgr.cc@315
PS2, Line 315: (*tmp_file)->AllocateSpace(scratch_range_bytes, file_offset);
Thanks for the cleanup, this makes it simpler.
--
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 17:26:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Attila Jeges, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9420
to look at the new patch set (#7).
Change subject: IMPALA-3866 Improve error reporting for scratch write errors
......................................................................
IMPALA-3866 Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
A be/src/runtime/io/disk-io-mgr-with-fault-injection.cc
A be/src/runtime/io/disk-io-mgr-with-fault-injection.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/errno-to-error-status-converter.cc
A be/src/runtime/io/errno-to-error-status-converter.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
12 files changed, 488 insertions(+), 76 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/7
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 7
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 16: Code-Review+2
--
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: 16
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 16:30:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
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:
(15 comments)
Thanks! Few more comments:
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG
Commit Message:
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.
> I observed this behaviour during the investigation and seemed a good idea t
Thanks, I was just curious if it was a general cleanup or something related to the JIRA proper.
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@384
PS2, Line 384: ExecuteWriteFailureTest
> The reason I implemented this way is that In case I followed how InvalidWri
Thanks, I think I got it now :)
http://gerrit.cloudera.org:8080/#/c/9420/4/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/4/be/src/runtime/io/disk-io-mgr-test.cc@188
PS4, Line 188: unique_ptr<B
nit: 'phase_to_fail'
http://gerrit.cloudera.org:8080/#/c/9420/4/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/4/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@53
PS4, Line 53:
const string& ?
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@60
PS4, Line 60:
Have you considered adding a public member function to DiskIoMgrWithFaultInjection class to set/clear 'fault_injection_to_write_' ?
Using the class like this is a bit awkward.
Also probably DiskIoMgrTest would not have to be a friend class then.
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr.h
File be/src/runtime/io/disk-io-mgr.h:
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr.h@402
PS4, Line 402: st in
nit: returned
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".
> I was thinking of this as well. The reason I chose strings is that I'd like
If you add a public member function to this class to set/clear 'faultInjectionToWrite_' (as I suggested), then passing in an enum parameter to that function would be more straightforward. You still need to map the enum literals to strings.
On the other hand it might be an overkill. Your call ;)
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@812
PS2, Line 812: DebugFaultInjection
> This is the point where I set errno to some desired value. Once it is set,
Thanks, my bad. I meant to comment this on GetErrorStatusFromErrno()
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr.cc@1107
PS4, Line 1107:
nit: parentheses are unnecessary.
http://gerrit.cloudera.org:8080/#/c/9420/4/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/4/be/src/runtime/io/errno-to-error-status-converter.h@43
PS4, Line 43: ror text r
Maybe this should be called 'errno_' or something similar to avoid confusing it with an Impala error code. Also, the comment above should refer to the formal parameter name. Please update it.
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.h@52
PS4, Line 52: function
Same here
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.h@56
PS4, Line 56:
Same here
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."}});
> Gathered the possible errno's from these pages:
Thanks
http://gerrit.cloudera.org:8080/#/c/9420/4/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/4/be/src/runtime/io/errno-to-error-status-converter.cc@54
PS4, Line 54: t_body)) {
Maybe this should be called 'errno_' or something similar to avoid confusing it with an Impala error code.
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.cc@76
PS4, Line 76:
Same as above
--
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: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Mar 2018 18:54:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 16:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2142/
--
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: 16
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 16:30:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Attila Jeges, Tim Armstrong, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9420
to look at the new patch set (#13).
Change subject: IMPALA-3866: Improve error reporting for scratch write errors
......................................................................
IMPALA-3866: Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/error-converter.cc
A be/src/runtime/io/error-converter.h
A be/src/runtime/io/local-file-system-with-fault-injection.cc
A be/src/runtime/io/local-file-system-with-fault-injection.h
A be/src/runtime/io/local-file-system.cc
A be/src/runtime/io/local-file-system.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/error-util.cc
M be/src/util/error-util.h
17 files changed, 621 insertions(+), 88 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/13
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 13
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 3:
Rebased with master + resolved conflicts.
--
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: 3
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Mar 2018 08:10:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Attila Jeges, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9420
to look at the new patch set (#11).
Change subject: IMPALA-3866: Improve error reporting for scratch write errors
......................................................................
IMPALA-3866: Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/error-converter.cc
A be/src/runtime/io/error-converter.h
A be/src/runtime/io/local-file-system-with-fault-injection.cc
A be/src/runtime/io/local-file-system-with-fault-injection.h
A be/src/runtime/io/local-file-system.cc
A be/src/runtime/io/local-file-system.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
15 files changed, 607 insertions(+), 85 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/11
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 11
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
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 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9420/4/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/4/be/src/runtime/io/errno-to-error-status-converter.cc@54
PS4, Line 54: error_code
> errno_ would indicate that this was some member of the object. I'd rather c
agree
--
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 <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Mar 2018 14:39:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Attila Jeges, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9420
to look at the new patch set (#8).
Change subject: IMPALA-3866: Improve error reporting for scratch write errors
......................................................................
IMPALA-3866: Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/error-converter.cc
A be/src/runtime/io/error-converter.h
A be/src/runtime/io/local-file-system-with-fault-injection.cc
A be/src/runtime/io/local-file-system-with-fault-injection.h
A be/src/runtime/io/local-file-system.cc
A be/src/runtime/io/local-file-system.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
15 files changed, 613 insertions(+), 85 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/8
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 2:
(5 comments)
Thx Attila for taking a look! Let me answer the ones I could from the top of my head and get back with the rest tomorrow(ish).
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG
Commit Message:
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?
I observed this behaviour during the investigation and seemed a good idea to make them void. Somewhat related to error message enhancement, but more of a dead code removal. Do you think I should leave them as they are?
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@384
PS2, Line 384: ExecuteWriteFailureTest
> Mayb it woyld be cleaner if you passed "/tmp/disk_io_mgr_test.txt" to Execu
The reason I implemented this way is that In case I followed how InvalidWrite executes 2 test then I would end up having 5 writes running in parallel and then I couldn't guarantee the execution order. ExecuteWriteFailureTest waits until the triggered read finishes.
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?
I was thinking of this as well. The reason I chose strings is that I'd like to include the function name to the error text, so in case I had an enum I would also need some map to translate it to string, right?
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 i
Seems a good idea not to mess the prod code for fault injection. I'll give this a try, thx!
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?
Gathered the possible errno's from these pages:
http://pubs.opengroup.org/onlinepubs/009696699/functions/open.html
http://pubs.opengroup.org/onlinepubs/009696699/functions/fdopen.html
http://pubs.opengroup.org/onlinepubs/009696699/functions/fseek.html
http://pubs.opengroup.org/onlinepubs/009696699/functions/fwrite.html
http://pubs.opengroup.org/onlinepubs/009696699/functions/fclose.html
I also collected inputs from Tim and Jeszy about these. I sent the related doc.
--
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: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 17:29:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9420 )
Change subject: IMPALA-3866 Improve error reporting for scratch write errors
......................................................................
IMPALA-3866 Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are anhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level function and an
error message for displaying purposes. Once any of these functions
fail than the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned funtions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/errno-to-error-status-converter.cc
A be/src/runtime/io/errno-to-error-status-converter.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
10 files changed, 322 insertions(+), 74 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/2
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Attila Jeges, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9420
to look at the new patch set (#6).
Change subject: IMPALA-3866 Improve error reporting for scratch write errors
......................................................................
IMPALA-3866 Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
A be/src/runtime/io/disk-io-mgr-with-fault-injection.cc
A be/src/runtime/io/disk-io-mgr-with-fault-injection.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/errno-to-error-status-converter.cc
A be/src/runtime/io/errno-to-error-status-converter.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
12 files changed, 488 insertions(+), 76 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/6
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 6
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9420/10/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/10/be/src/runtime/io/disk-io-mgr.cc@1098
PS10, Line 1098: if (!ret_status.ok()) goto end;
Hmm. Actually I think there was a pre-existing bug here. We should close fd if fdopen() fails. It's probably rare but according to fdopen() docs it is possible.
How about we move the open()/fdopen() sequence into a single method in LocalFileSystem, e.g. OpenForWrite(). Then we can hide the details of the error handling and also the raw file descriptor inside that function.
--
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: 10
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 20:16:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9420 )
Change subject: IMPALA-3866: Improve error reporting for scratch write errors
......................................................................
IMPALA-3866: Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Reviewed-on: http://gerrit.cloudera.org:8080/9420
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/error-converter.cc
A be/src/runtime/io/error-converter.h
A be/src/runtime/io/local-file-system-with-fault-injection.cc
A be/src/runtime/io/local-file-system-with-fault-injection.h
A be/src/runtime/io/local-file-system.cc
A be/src/runtime/io/local-file-system.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/error-util.cc
M be/src/util/error-util.h
17 files changed, 624 insertions(+), 88 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 17
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 16: Verified+1
--
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: 16
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 20:31:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Attila Jeges, Tim Armstrong, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9420
to look at the new patch set (#14).
Change subject: IMPALA-3866: Improve error reporting for scratch write errors
......................................................................
IMPALA-3866: Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/error-converter.cc
A be/src/runtime/io/error-converter.h
A be/src/runtime/io/local-file-system-with-fault-injection.cc
A be/src/runtime/io/local-file-system-with-fault-injection.h
A be/src/runtime/io/local-file-system.cc
A be/src/runtime/io/local-file-system.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/error-util.cc
M be/src/util/error-util.h
17 files changed, 624 insertions(+), 88 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/14
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 14
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht 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 12:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h
File be/src/runtime/io/error-converter.h:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h@41
PS12, Line 41: /// text is provided by the errno_to_error_text_map_ member.
it'd be good to comment on 'param's in the public interface (like you do below). I wasn't sure what that was until I read the private methods.
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc
File be/src/runtime/io/error-converter.cc:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@44
PS12, Line 44: {ENXIO, "Device doesn't exist."}});
out of curiosity, why do we define these rather than using GetStrErrMsg() all the time?
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@61
PS12, Line 61: GetStrErrMsg
GetStrErrMsg() has a comment about saving errno early or else it might get overwritten. How can we be sure errno still contains the original error by now, given this function may be called deeper in the call chain since err_no is passed through params?
--
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: 12
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Mar 2018 23:54:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht 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 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc
File be/src/runtime/io/error-converter.cc:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@44
PS12, Line 44: {ENXIO, "Device doesn't exist."}});
> The request was to enrich the basic error messages from GetStrErrMsg() so t
Okay. I thought GetStrErrMsg() gives similar text, but maybe not.
Is the motivation for this documented somewhere? If no, how about putting a quick comment explaining why we have this rather than just using GetStrErrMsg() since I don't think that's obvious.
--
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: 13
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 16:09:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 12:
(2 comments)
Thanks, I think this worked out quite nicely.
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h
File be/src/runtime/io/error-converter.h:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h@19
PS12, Line 19: IMPALA_RUNTIME_IO_ERRNO_TO_ERROR_STATUS_CONVERTER_H
needs update
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/local-file-system.h
File be/src/runtime/io/local-file-system.h:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/local-file-system.h@33
PS12, Line 33: Status OpenForWrite(const char* file_name, int option1, int option2, FILE** file);
I think this needs a comment explaining option1 and option2. Maybe it would be clearer if the were oflags and mode to match how it's described here: http://pubs.opengroup.org/onlinepubs/009696699/functions/open.html
--
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: 12
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Mar 2018 21:15:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 9:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.h
File be/src/runtime/io/disk-io-mgr.h:
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.h@373
PS8, Line 373: // Function to change the underlying LocalFileSystem object used for disk I/O.
> Let's make it clear that it's a test-only function.
Sure, Done
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.h@425
PS8, Line 425:
> We try to avoid shared_ptr as much as possible because shared ownership is
Sure, it works with unique_ptr as well. One thing is that I had to do a move() when calling the setter function and also one move() inside when actually setting the member.
I tried passing the ptr as ref but apparently it doesn't handle polymorphism well (accepts unique_ptr<LocalFileSystem> but given unique_ptr<LocalFileSystemWithFaultInjection> doesn't compile).
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.cc@1101
PS8, Line 1101:
> I missed on my first pass that we aren't fclose()ing the file when we hit a
As I see in the old code fclose() was invoked when the file_handle wasn't nullptr. So In case open() or fdopen() failed only HandleWriteFinished() was called without fclose(). So this results that fclose() is invoked only if both open() and fdopen() succeeds and no need to add fclose() to their error handling.
For WriteRangeHelper() the fclose() have to be run, so that is a bug in my current code, thanks!
Anyway, I haven't written gotos in years, let me do a small refactor here to fill this gap :)
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:
> Yeah that's a bit of an unfortunate case. We started off with boost::unorde
Thanks for the nice explanation!
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system-with-fault-injection.cc
File be/src/runtime/io/local-file-system-with-fault-injection.cc:
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system-with-fault-injection.cc@31
PS8, Line 31: return LocalFileSystem::OpenAux(file, option1, option2);
> Long lines. It might be useful to run clang-format on the patches to detect
Thanks for the advice!
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system.cc
File be/src/runtime/io/local-file-system.cc:
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system.cc@57
PS8, Line 57: Status LocalFileSystem::Fseek(FILE* file_handle, off_t offset, int whence,
> I'm not sure what this TODO means
Ohh, my bad. This is some leftover I overlooked. Removed.
--
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: 9
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 10:32:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Mar 2018 15:10:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9420/5/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/5/be/src/runtime/io/disk-io-mgr-test.cc@191
PS5, Line 191: const string& function_name, int err_no,
> nit: It would be nice to make formal parameter names consistent across func
Done
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-test.cc@350
PS5, Line 350: const string& function_name, int err_no,
> same as above
Done
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-test.cc@378
PS5, Line 378: file_nam
> nit: 'file_name' to be consistent with the formal parameter name in Execute
Done
http://gerrit.cloudera.org:8080/#/c/9420/5/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/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@56
PS5, Line 56: eFault
> nit: Maybe this should be called 'err_no_' for consistency?
Hmm. err_no_ just doesn't look that pretty for for me. May I leave this as it is?
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc
File be/src/runtime/io/disk-io-mgr-with-fault-injection.cc:
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@38
PS5, Line 38: ction("op
> Another approach would be to abstract away the open() syscall itself, inste
Done
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@38
PS5, Line 38: ugFaultInjection("open")) { return -1; }
> Not sure about this, but maybe the order of operands should be reversed (he
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: 6
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Mar 2018 07:55:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 7:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.h
File be/src/runtime/io/disk-io-mgr.h:
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.h@373
PS8, Line 373: /// queued buffers (plus the buffer that is returned to the client) gives good
Let's make it clear that it's a test-only function.
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.h@425
PS8, Line 425: /// provide a MemTracker.
We try to avoid shared_ptr as much as possible because shared ownership is generally harder to reason about. I think in this case unique_ptr is sufficient if you move() it into set_local_file_system
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.cc@1101
PS8, Line 1101: DCHECK(write_range != nullptr);
I missed on my first pass that we aren't fclose()ing the file when we hit an error now. It would be ok to add it before each call to HandleWriteFinished(), but I would probably use the "goto end" pattern since we need to execute the exact same two lines of code on all exit paths, i.e.:
ret_status = ...;
if (!ret_status.ok()) goto end:
...
end:
ret_status = local_file_system_->Fclose(file_handle, write_range);
HandleWriteFinished(writer_context, write_range, ret_status);
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: using std::unordered_map;
> I wasn't aware of this common/names.h, thx!
Yeah that's a bit of an unfortunate case. We started off with boost::unordered_map. In most cases where a std:: equivalent appeared we've automatically switched, but there was some concern here that the std:: versions of the maps would use somewhat more memory and be slower because the standard requires using doubly-linked lists for buckets: http://bannalia.blogspot.co.uk/2013/10/implementation-of-c-unordered.html
We could probably benchmark std::unordered_map and switch over if it looked acceptable but we haven't put the time in yet.
I'd suggest using std::unordered_map here since the map is small and its not perf-critical.
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system-with-fault-injection.cc
File be/src/runtime/io/local-file-system-with-fault-injection.cc:
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system-with-fault-injection.cc@31
PS8, Line 31:
Long lines. It might be useful to run clang-format on the patches to detect some of these minor issues:
https://wiki.cloudera.com/pages/viewpage.action?pageId=24646528
The downside is that clang-format is sometimes overly aggressive at reformatting surrounding lines, which adds unnecessary code churn.
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system.cc
File be/src/runtime/io/local-file-system.cc:
http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system.cc@57
PS8, Line 57:
I'm not sure what this TODO means
--
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: 7
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Mar 2018 21:45:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9420/10/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/10/be/src/runtime/io/disk-io-mgr.cc@1098
PS10, Line 1098: if (!ret_status.ok()) goto end;
> Nice catch!
Yeah that does seem like overkill to me too.
--
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: 10
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Mar 2018 21:16:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Attila Jeges, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9420
to look at the new patch set (#9).
Change subject: IMPALA-3866: Improve error reporting for scratch write errors
......................................................................
IMPALA-3866: Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/error-converter.cc
A be/src/runtime/io/error-converter.h
A be/src/runtime/io/local-file-system-with-fault-injection.cc
A be/src/runtime/io/local-file-system-with-fault-injection.h
A be/src/runtime/io/local-file-system.cc
A be/src/runtime/io/local-file-system.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
15 files changed, 608 insertions(+), 86 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/9
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 9
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 5:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/9420/4/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/4/be/src/runtime/io/disk-io-mgr-test.cc@188
PS4, Line 188: 'phase_to_fai
> nit: 'phase_to_fail'
Done
http://gerrit.cloudera.org:8080/#/c/9420/4/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/4/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@53
PS4, Line 53: const
> const string& ?
Done
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@60
PS4, Line 60: // DiskIoMgr::Write(). The user who sets this member is also respon
> Have you considered adding a public member function to DiskIoMgrWithFaultIn
My plan was to implement this fault injection logic writing as less code as possible even if it requires some 'awkward' decisions.
However, adding public members to set/reset the injection member is not much extra work so I gladly implement these.
Done
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr.h
File be/src/runtime/io/disk-io-mgr.h:
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr.h@402
PS4, Line 402: retur
> nit: returned
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@440
PS2, Line 440:
: /// The minimum
> If you add a public member function to this class to set/clear 'faultInject
I gave it a try. Started implementing the enum approach, but then realised that both my new classes would use it that would make it a bit more complicated to implement (move enum to separate class, or leavi it in one class but then the orhet class should use from there that wouldn't be elegant either). It's definitely feasible, however, I feel that it's an overkill considering that the functionality that requires this is only for testing purposes.
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr.cc@1107
PS4, Line 1107: !
> nit: parentheses are unnecessary.
Done
http://gerrit.cloudera.org:8080/#/c/9420/4/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/4/be/src/runtime/io/errno-to-error-status-converter.h@43
PS4, Line 43: err_no, co
> Maybe this should be called 'errno_' or something similar to avoid confusin
Done
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.h@52
PS4, Line 52: err_no, Pa
> Same here
Done
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.h@56
PS4, Line 56: err_no);
> Same here
Done
http://gerrit.cloudera.org:8080/#/c/9420/4/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/4/be/src/runtime/io/errno-to-error-status-converter.cc@54
PS4, Line 54: err_no, Pa
> Maybe this should be called 'errno_' or something similar to avoid confusin
errno_ would indicate that this was some member of the object. I'd rather choose err_no instead. What do you think?
Done
http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.cc@76
PS4, Line 76: err_no) {
> Same as above
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: 5
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Mar 2018 12:47:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc
File be/src/runtime/io/error-converter.cc:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@44
PS12, Line 44: {ENXIO, "Device doesn't exist."}});
> Okay. I thought GetStrErrMsg() gives similar text, but maybe not.
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: 14
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Mar 2018 07:46:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Attila Jeges, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9420
to look at the new patch set (#3).
Change subject: IMPALA-3866 Improve error reporting for scratch write errors
......................................................................
IMPALA-3866 Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are anhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level function and an
error message for displaying purposes. Once any of these functions
fail than the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned funtions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/errno-to-error-status-converter.cc
A be/src/runtime/io/errno-to-error-status-converter.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
10 files changed, 322 insertions(+), 75 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/3
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 13:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h
File be/src/runtime/io/error-converter.h:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h@19
PS12, Line 19: IMPALA_RUNTIME_IO_ERROR_CONVERTER_H
> needs update
Thx, I don't know how I could have missed this.
Done
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h@41
PS12, Line 41: /// text is provided by the errno_to_error_text_map_ member. The key-value pairs in
> it'd be good to comment on 'param's in the public interface (like you do be
Good point. I'll move the comment from the private method here as I don't see the point of having it for both functions.
Done
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc
File be/src/runtime/io/error-converter.cc:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@44
PS12, Line 44: {ENXIO, "Device doesn't exist."}});
> out of curiosity, why do we define these rather than using GetStrErrMsg() a
The request was to enrich the basic error messages from GetStrErrMsg() so that support can more easily identify the underlying issue for a scratch write error.
These custom error messages are the result of the consultation with Jeszy and Tim.
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@61
PS12, Line 61: GetStrErrMsg
> GetStrErrMsg() has a comment about saving errno early or else it might get
Good point. Made a second version of GetStrErrMsg() that receives the error code to make sure it's not overriden.
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/local-file-system.h
File be/src/runtime/io/local-file-system.h:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/local-file-system.h@33
PS12, Line 33: // A wrapper around open() and fdopen(). For the possible values of oflag and mode
> I think this needs a comment explaining option1 and option2. Maybe it would
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: 13
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 13:19:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht 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 15: Code-Review+2
--
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: 15
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Mar 2018 22:50:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 15: Code-Review+1
(1 comment)
Will let Dan +2. Looks like you addressed his comments.
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h
File be/src/runtime/io/error-converter.h:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h@19
PS12, Line 19: IMPALA_RUNTIME_IO_ERROR_CONVERTER_H
> Thx, I don't know how I could have missed this.
I'm pretty sure there are other cases in the codebase where its inconsistent or screwy. Not a big deal, just trying to minimise entropy in the code.
--
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: 15
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Mar 2018 18:40:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
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 6:
(7 comments)
Thanks!
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc
File be/src/runtime/io/disk-io-mgr-with-fault-injection.cc:
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@38
PS6, Line 38: {
nit: Parentheses not necessary (here and below).
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@52
PS6, Line 52: int64_t
fwrite() retuns size_t
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@53
PS6, Line 53: return -1;
fwrite() should return 0 in case of failure.
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@58
PS6, Line 58: return -1;
fclose() should return EOF in case of failure.
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc@1099
PS6, Line 1099: int64_t
fwrite() returns size_t
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc@1099
PS6, Line 1099: WriteRange*
const WriteRange* ?
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc@1106
PS6, Line 1106: file_handle
file_handle != nullptr
--
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: 6
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Mar 2018 13:26:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
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 7: Code-Review+1
Thanks
--
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: 7
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Mar 2018 15:28:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
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 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9420/10/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/10/be/src/runtime/io/disk-io-mgr.cc@1098
PS10, Line 1098: if (ret_status.ok() && !close_status.ok()) ret_status = close_status;
> Hmm. Actually I think there was a pre-existing bug here. We should close fd
Nice catch!
Done
Note, that I considered close() also to be fault injected, but then for testing I would have needed a way to make both fdopen() and close() fail but the current implementation of the fault injection can handle only one of them. Could have implemented a way to handle multiple failures as well but found that an overkill comparing to the logic this is meant to cover.
What do you think?
--
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: 12
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Mar 2018 11:29:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Attila Jeges, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9420
to look at the new patch set (#12).
Change subject: IMPALA-3866: Improve error reporting for scratch write errors
......................................................................
IMPALA-3866: Improve error reporting for scratch write errors
The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.
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.
For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.
Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/error-converter.cc
A be/src/runtime/io/error-converter.h
A be/src/runtime/io/local-file-system-with-fault-injection.cc
A be/src/runtime/io/local-file-system-with-fault-injection.h
A be/src/runtime/io/local-file-system.cc
A be/src/runtime/io/local-file-system.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
15 files changed, 602 insertions(+), 86 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/12
--
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: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 12
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>