Adar Dembo 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 into the scan (i.e. iterator) path really that bad? Seems to me like it'd be a reasonable one-time cost, after which adding a new argument is just modifying the IOContext. Besides general ugliness, using a threadlocal means: 1. IOContext needs shared ownership and cannot be a simple stack-allocated object. 2. Any "thunking" of execution from one thread to another needs to set up CurrentIOContext() in the destination thread. If IOContext were plumbed simply, you could make a copy of it, or retain the shared ownership model and pass a ref. 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 abstraction is to pass arbitrary data, not just the tablet ID, right? For two, what exactly is less visible here? 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. 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 constructor? 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 performing IO for a tablet need to preempt itself temporarily for another tablet? -- 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 18:28:55 +0000 Gerrit-HasComments: Yes
