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 <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: Mon, 12 Mar 2018 21:45:01 +0000 Gerrit-HasComments: Yes
