Uploaded image for project: 'Couchbase Server'
  1. Couchbase Server
  2. MB-40577

Dropping a dataset with an non-existent dataverse returns error 24034 (cannot find dataverse) in 6.6, but a "cannot find dataset" error in 6.5

    XMLWordPrintable

    Details

    • Triage:
      Untriaged
    • Story Points:
      1
    • Is this a Regression?:
      Yes
    • Sprint:
      CX Sprint 209

      Description

      We (Java SDK folk) are now running our CI tests against 6.6, which has found this minor regression.

      The test is:

      @Test
      void dropDatasetFailsIfAbsent() {
          assertThrows(DatasetNotFoundException.class, () -> analytics.dropDataset("foo",
            dropDatasetAnalyticsOptions()
                .dataverseName("absentDataverse")));
      }
       

      In 6.6 this now fails as it raises a DataverseNotFoundException rather than DatasetNotFoundException, as cbas now returns a 24034 code.  In this test and the one below too, neither "foo" nor "absentDataverse" exist.

      This causes problems for our `ignoreIfNotExist` option, which allows dropping datasets and silently ignoring if they don't exist.  Because that logic is expecting a DataverseNotFoundException. 

      E.g. this other test fails with a DataverseNotFoundException, when you would expect it to succeed.  This test also passed in 6.5.

      @Test
      void dropDatasetCanIgnoreAbsent() {
        analytics.dropDataset("foo",
            dropDatasetAnalyticsOptions()
                .ignoreIfNotExists(true)
                .dataverseName("absentDataverse"));
      }
       

       For both tests, if I don't specify a dataverseName then the tests pass as expected.

        Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

          Hide
          dmitry.lychagin Dmitry Lychagin added a comment -

          This behavior was changed by https://issues.couchbase.com/browse/MB-39142 , specifically:
          https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/6203/8/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java#1576

          Graham Pople,

          We discussed this issue today and believe the new error code better explains the problem to the end user because it's more specific ("dataverse not found" instead of "dataset not found"). 

          Could you consider fixing it on your side?  If ignoreIfNotExists=true then do not fail on either DataverseNotFoundException or DatasetNotFoundException?

          Show
          dmitry.lychagin Dmitry Lychagin added a comment - This behavior was changed by  https://issues.couchbase.com/browse/MB-39142  , specifically: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/6203/8/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java#1576 Graham Pople , We discussed this issue today and believe the new error code better explains the problem to the end user because it's more specific ("dataverse not found" instead of "dataset not found").  Could you consider fixing it on your side?  If ignoreIfNotExists=true then do not fail on either DataverseNotFoundException or DatasetNotFoundException?
          Hide
          graham.pople Graham Pople added a comment -

          Dmitry, thanks for the reply.  I maybe agree with you on the specificity being better, but it is awkward for us to fix our side due to the changes needing to go into multiple SDKs, plus we need to update our sdk-rfcs detailing the protocol.  I will raise with the team and get back to you.

          Show
          graham.pople Graham Pople added a comment - Dmitry, thanks for the reply.  I maybe agree with you on the specificity being better, but it is awkward for us to fix our side due to the changes needing to go into multiple SDKs, plus we need to update our sdk-rfcs detailing the protocol.  I will raise with the team and get back to you.
          Hide
          graham.pople Graham Pople added a comment -

          Actually, I need to revisit the problem description.  I think there are two separate problems here.  Problem 1 is that this code:

          analytics.dropDataset("absentDataset", dropDatasetAnalyticsOptions().dataverseName("absentDataverse")) 

          used to raise DatasetNotFoundException and now raises DataverseNotFoundException. 

          I saw the ignoreIfNotExists test was failing also and assumed it was caused by the same change.  I'm rusty on the analytics manager code and thought it worked the same as N1QL, where we implement ignoreIfNotExists by ignoring a specific error code.  But analytics is different, it supports "IF EXISTS" syntax here, which we use. 

          So in fact I think there is a separate problem/regression 2 here, where

          analytics.dropDataset("absentDataset", dropDatasetAnalyticsOptions().ignoreIfNotExists().dataverseName("absentDataverse")) 

          which results in a single statement with "IF EXISTS" being sent to cbas, used to return success but now returns an error code causing us to raise DataverseNotFoundException. 

          If all that makes sense?

           

          So to clarify, the ask from analytics team is that all SDKs:

          1. For problem 1: I'm not sure?  Make no change and just accept that this is a minor breaking change for users in this edge case?

          2. For problem 2: ignore the 24034 (dataverse not found) error code if doing a "DROP DATASET ... IF EXISTS" statement?

          Show
          graham.pople Graham Pople added a comment - Actually, I need to revisit the problem description.  I think there are two separate problems here.  Problem 1 is that this code: analytics.dropDataset( "absentDataset" , dropDatasetAnalyticsOptions().dataverseName( "absentDataverse" )) used to raise DatasetNotFoundException and now raises DataverseNotFoundException.  I saw the ignoreIfNotExists test was failing also and assumed it was caused by the same change.  I'm rusty on the analytics manager code and thought it worked the same as N1QL, where we implement ignoreIfNotExists by ignoring a specific error code.  But analytics is different, it supports "IF EXISTS" syntax here, which we use.  So in fact I think there is a separate problem/regression 2 here, where analytics.dropDataset( "absentDataset" , dropDatasetAnalyticsOptions().ignoreIfNotExists().dataverseName( "absentDataverse" )) which results in a single statement with "IF EXISTS" being sent to cbas, used to return success but now returns an error code causing us to raise DataverseNotFoundException.  If all that makes sense?   So to clarify, the ask from analytics team is that all SDKs: 1. For problem 1: I'm not sure?  Make no change and just accept that this is a minor breaking change for users in this edge case? 2. For problem 2: ignore the 24034 (dataverse not found) error code if doing a "DROP DATASET ... IF EXISTS" statement?
          Hide
          dmitry.lychagin Dmitry Lychagin added a comment -

          Graham Pople,

          The change in IF EXISTS behavior (problem 2) is a regression. We're working on fixing this in 6.6.

          As for problem 1, our proposal is to leave it as is. So if user does not request "ignoreIfNotExists" and the dataverse does not exist, then the user will get DataverseNotFoundException instead of previous DatasetNotFoundException.

          Show
          dmitry.lychagin Dmitry Lychagin added a comment - Graham Pople , The change in IF EXISTS behavior (problem 2) is a regression. We're working on fixing this in 6.6. As for problem 1, our proposal is to leave it as is. So if user does not request "ignoreIfNotExists" and the dataverse does not exist, then the user will get DataverseNotFoundException instead of previous DatasetNotFoundException.
          Hide
          graham.pople Graham Pople added a comment -

          Ok Dmitry Lychagin, I'll let the team know and let you know if they have any feedback.  That sounds ok to me though, I'll modify these tests accordingly.  MB-40576 is more of a problem as we'll have to find some way of disabling the analytics tests, or a subset of them, but only on 6.6.0.

          Show
          graham.pople Graham Pople added a comment - Ok Dmitry Lychagin , I'll let the team know and let you know if they have any feedback.  That sounds ok to me though, I'll modify these tests accordingly.   MB-40576 is more of a problem as we'll have to find some way of disabling the analytics tests, or a subset of them, but only on 6.6.0.
          Hide
          build-team Couchbase Build Team added a comment -

          Build couchbase-server-6.6.0-7896 contains cbas-core commit c23928d with commit message:
          MB-40577: DROP DATASET IF EXISTS must not fail if dataverse not found

          Show
          build-team Couchbase Build Team added a comment - Build couchbase-server-6.6.0-7896 contains cbas-core commit c23928d with commit message: MB-40577 : DROP DATASET IF EXISTS must not fail if dataverse not found
          Hide
          will.broadbelt Will Broadbelt added a comment -

          Verified tests pass with 6.6.0-7897.

          Show
          will.broadbelt Will Broadbelt added a comment - Verified tests pass with 6.6.0-7897.
          Hide
          build-team Couchbase Build Team added a comment -

          Build couchbase-server-7.0.0-2746 contains cbas-core commit c23928d with commit message:
          MB-40577: DROP DATASET IF EXISTS must not fail if dataverse not found

          Show
          build-team Couchbase Build Team added a comment - Build couchbase-server-7.0.0-2746 contains cbas-core commit c23928d with commit message: MB-40577 : DROP DATASET IF EXISTS must not fail if dataverse not found
          Hide
          build-team Couchbase Build Team added a comment -

          Build couchbase-server-6.6.2-9599 contains cbas-core commit c23928d with commit message:
          MB-40577: DROP DATASET IF EXISTS must not fail if dataverse not found

          Show
          build-team Couchbase Build Team added a comment - Build couchbase-server-6.6.2-9599 contains cbas-core commit c23928d with commit message: MB-40577 : DROP DATASET IF EXISTS must not fail if dataverse not found

            People

            Assignee:
            will.broadbelt Will Broadbelt
            Reporter:
            graham.pople Graham Pople
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Due:
              Created:
              Updated:
              Resolved:

                Gerrit Reviews

                There are no open Gerrit changes

                  PagerDuty