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 <gaborkas...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Mar 2018 14:38:10 +0000
Gerrit-HasComments: Yes

Reply via email to