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

Reply via email to