Details

    • Type: Improvement
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 1.0.0
    • Fix Version/s: None
    • Component/s: docs
    • Security Level: Public
    • Labels:

      Description

      The docs don't mention that add_multi will raise KeyExistsError for the last already existing key, if any number of them already exist. If this is intentional behavior, mentioning it in the docs would be useful and that the KeyExistsError.all_results is where most people will need to look for the OperationResult list.

      This behavior seems odd for the API. Assuming there is not a connection/server error, always returning the OperationResult dict would be more intuitive and avoid the code pattern of:

      try:
      results = bucket.add_multi(keys)
      except KeyExistsError as e:

      1. ignore exception because I'm going to process the results anyway
        results = e.all_results
      1. now process/verify 'results'


      If add_multi always returned an OperationResult dict, the code would simply be:

      results = bucket.add_multi(keys)

      1. process/verify 'results' without the unnecessary exception
      No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

        Hide
        mnunberg Mark Nunberg added a comment -

        This may be fixed in subsequent versions, but the *multi API is expected to function identically to the single API with the exception of the return type being a dict rather than a Result object.

        What I may do in the future is simply make all exception throwing optional (i.e. at least those exceptions caused by libcouchbase - the library may still throw exceptions in true "exceptional" situations).

        Does that sound better?

        Show
        mnunberg Mark Nunberg added a comment - This may be fixed in subsequent versions, but the *multi API is expected to function identically to the single API with the exception of the return type being a dict rather than a Result object. What I may do in the future is simply make all exception throwing optional (i.e. at least those exceptions caused by libcouchbase - the library may still throw exceptions in true "exceptional" situations). Does that sound better?
        Hide
        manfre Michael Manfre added a comment -

        Given the API decision of "multi == single, except make the return type make sense for the function", how about changing that to "multi == single, except the return type and raised exceptions make sense for the function"? That API guideline change would open up two options to fix the multi functions.

        1. (Preferred Fix) Always return a dict for the multi functions and don't raise the key errors. This is how memcached handles get_multi. Errors should still be raised in true "exceptional" situations.

        2. Change the key error to also includes a list of keys that failed, instead of just all_results. While not ideal as always getting a result dict back, this would at least avoid all code from having to duplicate the work done by the couchbase driver, which already knows which keys failed.


        If you make all exception throwing optional, please do so on a per function basis and not on a per connection basis. Per connection settings are only useful when you control all code that may use the connection, which may prevent using 3rd party packages that made a different 'quiet' choice when writing their code.

        Show
        manfre Michael Manfre added a comment - Given the API decision of "multi == single, except make the return type make sense for the function", how about changing that to "multi == single, except the return type and raised exceptions make sense for the function"? That API guideline change would open up two options to fix the multi functions. 1. (Preferred Fix) Always return a dict for the multi functions and don't raise the key errors. This is how memcached handles get_multi. Errors should still be raised in true "exceptional" situations. 2. Change the key error to also includes a list of keys that failed, instead of just all_results. While not ideal as always getting a result dict back, this would at least avoid all code from having to duplicate the work done by the couchbase driver, which already knows which keys failed. If you make all exception throwing optional, please do so on a per function basis and not on a per connection basis. Per connection settings are only useful when you control all code that may use the connection, which may prevent using 3rd party packages that made a different 'quiet' choice when writing their code.
        Hide
        mnunberg Mark Nunberg added a comment -

        I'd like to get rid of exception throwing in all situations except for true "unrecoverable" exceptions (for example, invalid format or some other serious error). Otherwise I do agree.

        The problem with trying to figure out which keys "failed" is that currently only one exception (i.e. the first failure) is thrown - while the other ones still exist. Because Python cannot really return values when an exception has been thrown, those values are stored in the '.all_results' field of the exception object.

        Note that you can use the Item API (though this is not present in 1.0.0, it will be available in 1.1.0) to go back to 'normal' behavior.

        Also note that internally, the Couchbase driver doesn't really know which key failed - there is only one failure flag internally for key failures - so that there isn't a huge performance hit if a lot of keys fail.

        Feel free to rename this issue as "Don't raise exceptions on key failures", as this involves a lot of annoying boilerplate in any event. What we really should have done is something like:

        1. Raise on any error
          cb.set(key, value, raise=True)
        1. Raise only on specific failures
          cb.set(key, value, raise=[0x18, 0x27, 0x13])
        1. Don't raise (default)
          cb.set(key, value)
        Show
        mnunberg Mark Nunberg added a comment - I'd like to get rid of exception throwing in all situations except for true "unrecoverable" exceptions (for example, invalid format or some other serious error). Otherwise I do agree. The problem with trying to figure out which keys "failed" is that currently only one exception (i.e. the first failure) is thrown - while the other ones still exist. Because Python cannot really return values when an exception has been thrown, those values are stored in the '.all_results' field of the exception object. Note that you can use the Item API (though this is not present in 1.0.0, it will be available in 1.1.0) to go back to 'normal' behavior. Also note that internally, the Couchbase driver doesn't really know which key failed - there is only one failure flag internally for key failures - so that there isn't a huge performance hit if a lot of keys fail. Feel free to rename this issue as "Don't raise exceptions on key failures", as this involves a lot of annoying boilerplate in any event. What we really should have done is something like: Raise on any error cb.set(key, value, raise=True) Raise only on specific failures cb.set(key, value, raise= [0x18, 0x27, 0x13] ) Don't raise (default) cb.set(key, value)
        Hide
        mnunberg Mark Nunberg added a comment -

        This is a pretty old issue and I'm still revisiting. Unfortunately I don't have a good way of handling this without breaking the previous behavior. This might be implemented in a 3.0 version.

        Show
        mnunberg Mark Nunberg added a comment - This is a pretty old issue and I'm still revisiting. Unfortunately I don't have a good way of handling this without breaking the previous behavior. This might be implemented in a 3.0 version.

          People

          • Assignee:
            mnunberg Mark Nunberg
            Reporter:
            manfre Michael Manfre
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Gerrit Reviews

              There are no open Gerrit changes