Henrik Ingo <henrik.i...@avoinelama.fi> writes: > 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.
Defaulting to roughly number of CPU cores could be a good default. > 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.) I'd recommend looking at the http functions we already have, the BENCHMARK function and EXECUTE CONCURRENT. This should prove adequate for a basic slap-like test entirely from SQL. > @@ -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.) If we can easily get number of CPU cores, let's just launch that many threads. If each thread is for the large part non-blocking, then we're going to be pretty sweet with this as default. > -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.) probably the first multi-inheritance we have in drizzle :) > 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. I'd like if we did keep the version numbers... it allows us to have some form of backwards compat. e.g. if we remove 0.1/ because it's buggy and useless, and somebody has an app, a 404 error is much better than failing weirdly. I did it this way to begin with very much on purpose so that others could come along with much better API ideas and that any users could either continue to be happy with old APIs or get a very simple error message if we removed them. -- Stewart Smith
pgphbaRJ1oegP.pgp
Description: PGP signature
_______________________________________________ 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
