Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11248 )
Change subject: KUDU-2469 pt 1: add an IOContext ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h File src/kudu/fs/io_context.h: http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h@33 PS1, Line 33: // Utility class used to pass strings across many layers, avoiding excessive : // plumbing. > Ugh, why do we need to store IOContext in a threadlocal? Is the plumbing in You're right it's a one-time cost, though I leaned this way since the intended immediate use of the IOContext is error handling. It felt like overkill to explicitly plumb down things specifically for that. Though you raise a good point that the IOContext may in the future be used for more substantial things. http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h@39 PS1, Line 39: there is significantly less visibility into : // the string being passed. > Not sure I understand this. For one, the intention of the IOContext abstrac Right, it should be generalizable to anything, not just strings. It's less visible in that any arbitrary thread could call CurrentIOContext() and hope it gets something useful back. With the plumbing alternative, it's more obvious when something _should_ have an IOContext. http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h@42 PS1, Line 42: class IOContext : public RefCountedThreadSafe<IOContext> { > Please doc why IOContext needs to have shared ownership. Ack http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h@51 PS1, Line 51: void set_tablet_id(std::string tablet_id) { : tablet_id_ = std::move(tablet_id); : } > Why do we need the ability to manipulate tablet_id_ via both setter and con We don't, will remove. http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h@89 PS1, Line 89: IOContext::threadlocal_io_context_ = old_context_; > Is this "nesting" feature required? Under what circumstances does a thread It's not required, but I added it to match the "scoped" semantics we have elsewhere. An alternative would be to disallow such preemption, perhaps with a DCHECK or somesuch. -- To view, visit http://gerrit.cloudera.org:8080/11248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I645e10a2fda1f2564326b7052e6a0debc5ad738c Gerrit-Change-Number: 11248 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 16 Aug 2018 21:00:36 +0000 Gerrit-HasComments: Yes
