Uploaded image for project: 'Cassandra'
  1. Cassandra
  2. CASSANDRA-569

Add Thrift API to set per-connection settings

    Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Major
    • Resolution: Not A Problem
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      For thrift API, let you set configuration options per-connection. These would be stored thread-local on the server, and when a thrift connection dies, they would be reset.

      An example use case is setting the timeout for requests on a per-connection basis.

        Activity

        Hide
        jbellis Jonathan Ellis added a comment -

        Closing for lack of a use case.

        Show
        jbellis Jonathan Ellis added a comment - Closing for lack of a use case.
        Hide
        jbellis Jonathan Ellis added a comment -

        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?

        Show
        jbellis Jonathan Ellis added a comment - 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?
        Hide
        urandom Eric Evans added a comment -

        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?

        Show
        urandom Eric Evans added a comment - 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?
        Hide
        jbellis Jonathan Ellis added a comment -

        That makes sense, thanks.

        Show
        jbellis Jonathan Ellis added a comment - That makes sense, thanks.
        Hide
        pquerna Paul Querna added a comment -

        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.

        Show
        pquerna Paul Querna added a comment - 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.
        Hide
        jbellis Jonathan Ellis added a comment - - 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.

        Show
        jbellis Jonathan Ellis added a comment - - 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.
        Hide
        pquerna Paul Querna added a comment -

        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.

        Show
        pquerna Paul Querna added a comment - 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.

          People

          • Assignee:
            Unassigned
            Reporter:
            pquerna Paul Querna
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development