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

          graham.pople Graham Pople created issue -
          graham.pople Graham Pople made changes -
          Field Original Value New Value
          Priority Minor [ 4 ] Major [ 3 ]
          graham.pople Graham Pople made changes -
          Description We (Java SDK folk) are now running our CI tests against 6.6, which has found this minor regression.

          The test is:
          {code:java}
          @Test
          void dropDatasetFailsIfAbsent() {
              assertThrows(DatasetNotFoundException.class, () -> analytics.dropDataset("foo",
                dropDatasetAnalyticsOptions()
                    .dataverseName("absentDataverse")));
          }
           {code}
          In 6.6 this now fails as it raises a DataverseNotFoundException rather than DatasetNotFoundException, as cbas now returns a 24034 code.

          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 (neither "foo" nor "absentDataverse" exist).  This test also passed in 6.5.
          {code:java}
          @Test
          void dropDatasetCanIgnoreAbsent() {
            analytics.dropDataset("foo",
                dropDatasetAnalyticsOptions()
                    .ignoreIfNotExists(true)
                    .dataverseName("absentDataverse"));
          }
           {code}
           
          We (Java SDK folk) are now running our CI tests against 6.6, which has found this minor regression.

          The test is:
          {code:java}@Test
          void dropDatasetFailsIfAbsent() {
              assertThrows(DatasetNotFoundException.class, () -> analytics.dropDataset("foo",
                dropDatasetAnalyticsOptions()
                    .dataverseName("absentDataverse")));
          }
           {code}
          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.
          {code:java}@Test
          void dropDatasetCanIgnoreAbsent() {
            analytics.dropDataset("foo",
                dropDatasetAnalyticsOptions()
                    .ignoreIfNotExists(true)
                    .dataverseName("absentDataverse"));
          }
           {code}
           For both tests, if I don't specify a dataverseName then the tests pass as expected.
          till Till Westmann made changes -
          Rank Ranked higher
          till Till Westmann made changes -
          Affects Version/s 6.6.0 [ 16787 ]
          till Till Westmann made changes -
          Labels triaged
          till Till Westmann made changes -
          Assignee Till Westmann [ till ] Dmitry Lychagin [ dmitry.lychagin ]
          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?
          till Till Westmann made changes -
          Fix Version/s 6.6.0 [ 16787 ]
          till Till Westmann made changes -
          Is this a Regression? Unknown [ 10452 ] Yes [ 10450 ]
          dmitry.lychagin Dmitry Lychagin made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          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.
          till Till Westmann made changes -
          Link This issue blocks MB-38724 [ MB-38724 ]
          till Till Westmann made changes -
          Labels triaged approved-for-6.6.0 triaged
          till Till Westmann made changes -
          Due Date 24/Jul/20
          dmitry.lychagin Dmitry Lychagin made changes -
          Sprint CX Sprint 209 [ 1167 ]
          dmitry.lychagin Dmitry Lychagin made changes -
          Link This issue relates to MB-40592 [ MB-40592 ]
          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.
          dmitry.lychagin Dmitry Lychagin made changes -
          Resolution Fixed [ 1 ]
          Status In Progress [ 3 ] Resolved [ 5 ]
          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
          ritam.sharma Ritam Sharma made changes -
          Assignee Dmitry Lychagin [ dmitry.lychagin ] Arunkumar Senthilnathan [ arunkumar ]
          arunkumar Arunkumar Senthilnathan made changes -
          Labels approved-for-6.6.0 triaged approved-for-6.6.0 combination_testing triaged
          arunkumar Arunkumar Senthilnathan made changes -
          Assignee Arunkumar Senthilnathan [ arunkumar ] Will Broadbelt [ will.broadbelt ]
          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.
          will.broadbelt Will Broadbelt made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          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