Hi, Check this: http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-json_server-multithreading/revision/2572
On Mon, Jul 16, 2012 at 3:39 PM, Henrik Ingo <henrik.i...@avoinelama.fi> wrote: > On Sat, Jul 14, 2012 at 5:13 PM, Mohit Srivastava > <srivastavamohi...@gmail.com> wrote: >> http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-alsosql-keyvalue/revision/2575 > >> I am done with multi-threading and dynamic plugin part.New one for review >> :) > > > On Thu, Jul 12, 2012 at 11:06 PM, Henrik Ingo <henrik.i...@avoinelama.fi> > wrote: >> *** 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*) > > This is still not done. Done > > >> 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. > > I assume this was not done yet? (It's ok, just keeping track.) > Will do soon. >> *** "Known issues" - still todo: >> >> Ability to at least increase max_threads dynamically. > > Done. Please correct: > > + errmsg_printf(error::ERROR, _("json_server_max_thread cannot be > smaller than previous configured value"));//error > > Should be json_server_max_threads (plural) > > Also, use of errmsg_printf() here is not correct. This will print > error message to stderr, which will be directed to some log file. > However, here the user is setting this variable interactively, so we > should return a warning level message back to the user that his action > was ignored. (Note that returning error to the client is too hard. We > should inform the user that the action was ignored, but it shouldn't > be an error to try.) > > Please see how I'm using my_error() in plugin/js/js.cc to return an > error to the interactive client. Different forms of my_error() are > defined in drizzled/error.h > > To be honest, I do not know how to issue a warning to the user. (Ie > there is no function my_warning() and also my_message() is not > exported to be part of DRIZZLED_API. If you can't figure it out, you > can just leave the code as it is now, and add "todo: we should return > a warning to the client, not to stderr". > > Segmentation fault occurs. Above mentioned revision have commented code for this. > Unrelated - remove this comment. (You've removed the code it is referencing > :-) > > // Catch all does nothing and returns generic message. > > > >> >> Documentation. > > Ok. (I will again make small language enhancements and push a branch > you can merge.) > >> 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.) > > todo > >> >> 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. > > todo > > >> This behavior that drizzled immediately spawns max_threads number of >> threads (even if they are never used) should be documented in the >> manual. > > Ok. > > >> *** 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"; > > > Ok. > >> === 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 > > Ok. > >> static const bool ALLOW_DROP_TABLE=false; >> >> By the way: Also change this to DEFAULT_ALLOW_DROP_TABLE >> > > Ok. > > >> bool allow_drop_table; >> string default_schema; >> string default_table; >> +uint32_t max_thread; >> >> Change name to max_threads > > Ok. > > >> { >> private: >> - drizzled::thread_ptr json_thread; >> + std::vector<drizzled::thread_ptr> json_thread; >> >> Change name to json_threads (plural) >> > > Ok. > > >> + >> + // 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. >> > > Here you have missed the discussion we had with Stewart. The new directions > are: > > 1) Remove /0.1/sql, /0.2/sql and /0.2/json on the grounds that we have > found crashing bugs in the old code so it is no longer supported. > > 2) We still continue the approach to make functionality available via > a URL that starts with the version number. Hence, you need to now > support these urls: > /0.1/version * = see below > /0.2/version * = see below > /0.3/version > /0.3/sql > /0.3/json > /latest/version > /latest/sql > /latest/json > /version > /sql > /json > / > > * For these two, you should use the original function that was > actually used in those versions of the plugin: > > extern "C" void process_api01_version_req(struct evhttp_request *req, void* ) > { > struct evbuffer *buf = evbuffer_new(); > if (buf == NULL) return; > > Json::Value root; > root["version"]= ::drizzled::version(); > > Json::StyledWriter writer; > std::string output= writer.write(root); > > evbuffer_add(buf, output.c_str(), output.length()); > evhttp_send_reply(req, HTTP_OK, "OK", buf); > } > I just change the function . And it doesn't represent api now. Because /0.1/version ,/0.2/version will behave same. >> You need to update documentation to reflect this change. >> > > I have updated documentation so it reflects the above urls. > > >> **** Not in diff: >> >> You can now remove this todo at line 303: >> >> * @todo allow DBA to set default schema (also in post,del methods) > > Ok. > > >> 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); >> } >> > > You changed the other file descriptor to nfd instead. This is ok. > > > Please merge lp:~hingo/drizzle/drizzle-json_server-multithread-docs to > get my edits of the documentation. > > Please correct the URLs as explained above and the issues with the > updateMaxThreads() code. > > Please push all of that to a branch with a more descriptive name. Then > issue merge request. > > Please add the remaining todo items to the blueprint. > > 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
