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.
> 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.)
> *** "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".
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);
}
> 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
