----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review55568 -----------------------------------------------------------
Great iteration Kapil! This is your first major contribution to the code base so there are some other helpful things to point out (like our use of JSON to protobuf, see below), so after this next iteration I think we'll be very near to commit! src/examples/example_module_impl.cpp <https://reviews.apache.org/r/25848/#comment95926> To Bernd's point above, if you declare/define this at the bottom then you won't need the forward declarations up top. I like this pattern, define everything you need before the Module<T> declaration/definition, which will tend to come at the bottom of files! A bigger concern I have, however, is regarding the lifetime of these objects. What happens if/when the dynamic library gets closed? Or what about when the application gets shutdown? Are these subject to destructors/cleanup? If so, either we need to have module writers create Module<T>* or we need to do deep copies when we read the symbols the first time (which actually still won't be immune to potential destruction because an application might be shutting down right when we start to read the symbol which could cause a crash, probably not the worst thing since the application was already being shutdown but still not a great user experience). src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95999> Please put * next to type name, thanks! src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95939> One of the main reasons we use JSON is so that we can easily convert to and from typed data structures represented by protobufs in our C++ code. So I'd like to see a protobuf here. In general it makes working with JSON much cleaner (and we don't have to have wierd typedefs). Based on your current JSON I could see a protobuf looking something like (but if this doesn't match how you'd want to use it, then lets change the JSON!): message Modules { message Library { optional string path = 1; repeated string modules = 2; } repeated Library libraries = 1; } Then we'd add a 'parse' function in src/common/parse.hpp which is where the other generic flag parsing functions reside. Then ModulesManager::load should just take an instance of Modules, without needing to first parse a flag (the Flags infrastructure will automagically take care of calling the correct parse function to get an instance of Modules). Leads to a clean separation of concerns! And now we have a datatype that we can do things with cleanly in C++, like work with in the tests (where I've made some comments as well). src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95998> Use const string& and * on type name please, thanks! src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95994> To the best of my knowledge this is only being used to make sure that we keep our instance of DynamicLibrary from getting destructed (and thus closing the library). Maybe the idea was to at some point not reopen a dynamic library that had already been opened? But that doesn't appear to be happening now. At the very least we should document this, but I could also see us changing this around to be a hashmap<ModuleBase*, Shared<DynamicLibrary>> where each instance of ModuleBase that depends on that DynamicLibrary has an entry since this seems slightly less arbitrary than saving the "path" of the DynamicLibrary (which might not even be a path IIUC). Alternatively, maybe we skip the map all together here and just go with a data structure that stores all the DynamicLibrary instances (i.e., using a std::list?) and add a TODO that says make them addressable only if/when we decide to implement something that let's you remove a module (and thus close it's dynamic library). But heck, I'd even be okay with just putting the DynamicLibrary on the heap in 'load' without an Owned/Shared and putting a TODO and comment directly there explaining that currently we never delete the DynamicLibrary instance nor do we expose a way to delete it so for now we just let the pointer dangle! src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95995> Can we add a brief comment here that this is from a "module name" to the actual ModuleBase and that if two modules from different libraries have the same name then the last one specified in the protobuf Modules will be picked. This needs to clearly be documented for the modules writer as well, and is what will probably lead us into writing module names that are of the format org_apache_mesos_test. In fact, if this is our plan I wouldn't be opposed to starting this convention with our example modules too to set the precedent. src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95941> This seems unnecessarily severe. If someone makes a mistake constructing their module they've just caused the master/slave to crash. Why not return Error if these are NULL instead? src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95993> I've seen this a couple of times throughout this review, please use const & for the "left hand side" of the foreach* so that we can avoid a copy. Give a quick skim through the rest of this review for other places please, thanks! src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment95996> const & src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment95942> We can use the Modules object defined above rather than doing JSON construction ourself (which is usually code smell to me). Modules modules; Modules::Library* library = modules.add_libraries(); library->set_path(getLibraryPath("examplemodule")); library->add_modules("examplemodule"); ... JSON::Protobuf(modules) ... src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment95934> I see that you can't pass DynamicLibrary as a const& because loadSymbol is not const, but it really should be. We avoid passing non-const references where possible, so please pass a pointer here instead. Thanks! src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment95933> Why do you need functions for this? Why not simplify and just load the symbol you want to change and change it directly? Same applies below. I see Bernd commented that the use of these functions was hard to track down, so why not eliminate that function all together! ;-) - Benjamin Hindman On Oct. 3, 2014, 11:46 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25848/ > ----------------------------------------------------------- > > (Updated Oct. 3, 2014, 11:46 p.m.) > > > Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, > and Timothy St. Clair. > > > Bugs: MESOS-1384 > https://issues.apache.org/jira/browse/MESOS-1384 > > > Repository: mesos-git > > > Description > ------- > > Adding a first class primitive, abstraction and process for dynamic library > writing and loading can make it easier to extend inner workings of Mesos. > Making it possible to have dynamic loadable resource allocators, isolators, > containerizes, authenticators and much more. > > > Diffs > ----- > > include/mesos/module.hpp PRE-CREATION > src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724 > src/examples/example_module_impl.cpp PRE-CREATION > src/examples/test_module.hpp PRE-CREATION > src/examples/test_module_impl.cpp PRE-CREATION > src/module/manager.hpp PRE-CREATION > src/module/manager.cpp PRE-CREATION > src/tests/module_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/25848/diff/ > > > Testing > ------- > > Ran make check with added tests for verifying library/module loading and > simple version check. > > > Thanks, > > Kapil Arya > >
