Paul Querna
added a comment - 28/Nov/09 07:47 rough patch.
feedback welcome, especially on naming of the new method.
adds setopt_timeout() to the thrift api, and stores/users the setting in Thread local storage of DatabaseDescriptor.

Jonathan Ellis
added a comment - 30/Nov/09 15:54 - edited Thanks for the patch! A question:
Why do we need resetLocalSettings in getProcessor? Since we are using threaded server, shouldn't the threadlocalness handle that for us when a new client connects?
Also, since we are moving towards using DatabaseDescriptor for fat clients as well as servers (see CASSANDRA-535 ), StorageProxy is a better place for the threadlocal.

resetLocalSettings is called from getProcessor, because getProcessor is the only indication you have from thrift that a new client has connected. As far as I could see, there isnt a way to tell if a client is disconnected either.

Without this resetLocalSettings call, if a client set a low timeout, it disconnected, and then a new client connected, this new client using the same thread would have the low timeout – the reset is there to get all new clients back to the default timeouts.

From my quick reading of the Thrift Thread pool server, it does reuse threads, not just spawn one new thread per client.

Paul Querna
added a comment - 30/Nov/09 20:17 resetLocalSettings is called from getProcessor, because getProcessor is the only indication you have from thrift that a new client has connected. As far as I could see, there isnt a way to tell if a client is disconnected either.
Without this resetLocalSettings call, if a client set a low timeout, it disconnected, and then a new client connected, this new client using the same thread would have the low timeout – the reset is there to get all new clients back to the default timeouts.
From my quick reading of the Thrift Thread pool server, it does reuse threads, not just spawn one new thread per client.

I'd suggest set_option(<option>, <value>) for the thrift method, with an option name of "rpc timeout", which seems most consistent with what's already there, (see get_string_property and its property names, i.e. "config file", "cluster name", "token map").

Also, we should probably maintain DatabaseDescriptor's scope as that of static configuration access and move the threadlocal into StorageProxy with a getLocalRpcTimeout() method that can fall back to DatabaseDescriptor.getRpcTimeout() if no local one has been set.

Eric Evans
added a comment - 01/Dec/09 19:37 I'd suggest set_option(<option>, <value>) for the thrift method, with an option name of "rpc timeout", which seems most consistent with what's already there, (see get_string_property and its property names, i.e. "config file", "cluster name", "token map").
Also, we should probably maintain DatabaseDescriptor's scope as that of static configuration access and move the threadlocal into StorageProxy with a getLocalRpcTimeout() method that can fall back to DatabaseDescriptor.getRpcTimeout() if no local one has been set.
How does that sound?

Revisiting this, I find I don't understand the motivation for the change. What use case is this fixing? If most operations complete within time X, but a few need time X + Y, then why not just make the global timeout X + Y?

Jonathan Ellis
added a comment - 30/Dec/09 16:39 Revisiting this, I find I don't understand the motivation for the change. What use case is this fixing? If most operations complete within time X, but a few need time X + Y, then why not just make the global timeout X + Y?