Review of https://code.launchpad.net/~srivastavamohit91/drizzle/drizzle-alsosql-keyvalue Revisions 2570 .. 2573
*** Miscellaneous: Please use more descriptive name for the branch. "drizzle-json_server-multihtreading" would be a good choice here. (Or at least something with multithread*) Please file a bug about the issue we found in /sql api where multiple statements return result sets with different amount of columns and crash the server. *** "Known issues" - still todo: Ability to at least increase max_threads dynamically. Documentation. Optional: A shell script to actually test multi-threading. (shell script needs to handle forking, drizzle-test-run doesn't know anything about such...) (If you don't do this now, add a mention about this to the worklog so we can remember it later.) Optional: Instead of launching max_threads amount of threads at startup, launch at first only 10, and then launch more if the server is busy. If you don't do this now (we shouldn't, it's non-trivial), add a mention of this to the worklog so that we remember to do it some time later. This behavior that drizzled immediately spawns max_threads number of threads (even if they are never used) should be documented in the manual. *** diff starts here $ bzr diff -r2570..2573 Both http_server.h and http_server.cc: All files need to have a copyright header. In this case I think you can just insert the GPL header with your name. However, you should add a line just below it that says something like "Based on example code from https://gist.github.com/665437"; === modified file 'plugin/json_server/json_server.cc' --- plugin/json_server/json_server.cc 2012-07-06 11:41:21 +0000 +++ plugin/json_server/json_server.cc 2012-07-11 14:03:34 +0000 @@ -59,6 +59,7 @@ #include <plugin/json_server/json/json.h> #include <plugin/json_server/db_access.h> #include <plugin/json_server/http_handler.h> +#include <plugin/json_server/http_server.h> namespace po= boost::program_options; using namespace drizzled; @@ -71,12 +72,15 @@ static const string DEFAULT_SCHEMA = "test"; static const string DEFAULT_TABLE = ""; static const string JSON_SERVER_VERSION = "0.3"; +static const uint32_t DEFAULT_MAX_THREAD= 1; Use a higher default. Json server isn't loaded by default. So if it is loaded, we can assume it is going to be used. I would set this at 64. (It should be even higher, but then we'd need to implement something that creates more threads as needed, not just 1024 threads at startup.) Also: change name to DEFAULT_MAX_THREADS static const bool ALLOW_DROP_TABLE=false; By the way: Also change this to DEFAULT_ALLOW_DROP_TABLE bool allow_drop_table; string default_schema; string default_table; +uint32_t max_thread; Change name to max_threads -class JsonServer : public drizzled::plugin::Daemon +class JsonServer : public drizzled::plugin::Daemon , public HTTPServer Multi-inheritance isn't necessarily a good practice, but in this case we inherit exactly one method, so I'll look away. (Sometimes it's ok to bend the rules.) { private: - drizzled::thread_ptr json_thread; + std::vector<drizzled::thread_ptr> json_thread; Change name to json_threads (plural) + + // Create Max_thread number of threads. + for(uint32_t i =0;i<max_thread;i++) { - sql_perror("event_add"); - return false; + if ((base= event_init()) == NULL) + { + sql_perror("event_init()"); + return false; + } + + if ((httpd= evhttp_new(base)) == NULL) + { + sql_perror("evhttp_new()"); + return false; + } + + if(evhttp_accept_socket(httpd,fd)) + { + sql_perror("evhttp_accept_socket()"); + return false; + } + + // These URLs are available. Bind worker method to each of them. + // // Please group by api version. Also unchanged functions must be copied to next version! + evhttp_set_cb(httpd, "/", process_root_request, NULL); + // API 0.1 + evhttp_set_cb(httpd, "/0.1/version", process_api01_version_req, NULL); + evhttp_set_cb(httpd, "/0.1/sql", process_api01_sql_req, NULL); + // API 0.2 + evhttp_set_cb(httpd, "/0.2/version", process_api01_version_req, NULL); + evhttp_set_cb(httpd, "/0.2/sql", process_api01_sql_req, NULL); + evhttp_set_cb(httpd, "/0.2/json", process_api02_json_req, NULL); + // API "latest" and also available in top level + evhttp_set_cb(httpd, "/latest/version", process_api01_version_req, NULL); + evhttp_set_cb(httpd, "/latest/sql", process_api01_sql_req, NULL); + evhttp_set_cb(httpd, "/latest/json", process_api02_json_req, NULL); + evhttp_set_cb(httpd, "/version", process_api01_version_req, NULL); + evhttp_set_cb(httpd, "/sql", process_api01_sql_req, NULL); + evhttp_set_cb(httpd, "/json", process_api02_json_req, NULL); + // Catch all does nothing and returns generic message. + //evhttp_set_gencb(httpd, process_request, NULL); + I've been thinking you could just remove all the /0.1/* and /0.2/ and /latest/ URIs. We are not strictly leaving old versions of the functions in place (also because they had crashing bugs) so let's just simplify and just provide /version /sql and /json and nothing more. You need to update documentation to reflect this change. **** Not in diff: You can now remove this todo at line 303: * @todo allow DBA to set default schema (also in post,del methods) In this function, To avoid confusion, please change "fd" to "wakeup_fd": static void shutdown_event(int fd, short, void *arg) { struct event_base *base= (struct event_base *)arg; event_base_loopbreak(base); close(fd); } henrik -- henrik.i...@avoinelama.fi +358-40-8211286 skype: henrik.ingo irc: hingo www.openlife.cc My LinkedIn profile: http://www.linkedin.com/profile/view?id=9522559 _______________________________________________ Mailing list: https://launchpad.net/~drizzle-discuss Post to : drizzle-discuss@lists.launchpad.net Unsubscribe : https://launchpad.net/~drizzle-discuss More help : https://help.launchpad.net/ListHelp
