Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.98.0, 0.95.2, 0.94.11
    • Component/s: Thrift
    • Labels:
      None
    • Tags:
      Thrift2

      Description

      Introduce new enum TDurability to expose more options for Write To Wal.

      1. 8947.revert.txt
        59 kB
        stack
      2. HBASE-8947.patch
        46 kB
        Hamed Madani
      3. HBASE-8947-v2.patch
        46 kB
        Hamed Madani
      4. HBASE-8947-v3.patch
        50 kB
        Hamed Madani
      5. HBASE-8947-v3-0.94.patch
        35 kB
        Hamed Madani
      6. HBASE-8947-v4-.094.patch
        28 kB
        Hamed Madani
      7. HBASE-8947-v4.patch
        62 kB
        Hamed Madani

        Activity

        Hide
        Hamed Madani added a comment -

        Missed FSYNC_WAL. Updated the patch and Added FSYNC_WAL to TDurability enum.

        Show
        Hamed Madani added a comment - Missed FSYNC_WAL. Updated the patch and Added FSYNC_WAL to TDurability enum.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12592272/HBASE-8947.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12592272/HBASE-8947.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6335//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12592275/HBASE-8947-v2.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12592275/HBASE-8947-v2.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6336//console This message is automatically generated.
        Hide
        Lars George added a comment -

        Hi Hamed Madani, I have two issues here, first a simple one: in hbase.thrift you have under "Specify Durability" twice SKIP_WAL, where for one you meant write SYNC_WAL.

        The more major issue I have is that the patch as it is is breaking compatibility. We should add TDurability, but not replace writeToWAL. In other words, keep the existing, add the new with a new index, and then in the code use isSetDurability() to check if the client is using it and prefer it over the existing Boolean. In case the existing Boolean is used, use the isSetWriteToWAL() and translate it into the SYNC_WAL or SKIP_WAL durability value as needed. Make sense?

        Show
        Lars George added a comment - Hi Hamed Madani , I have two issues here, first a simple one: in hbase.thrift you have under "Specify Durability" twice SKIP_WAL, where for one you meant write SYNC_WAL. The more major issue I have is that the patch as it is is breaking compatibility. We should add TDurability, but not replace writeToWAL. In other words, keep the existing, add the new with a new index, and then in the code use isSetDurability() to check if the client is using it and prefer it over the existing Boolean. In case the existing Boolean is used, use the isSetWriteToWAL() and translate it into the SYNC_WAL or SKIP_WAL durability value as needed. Make sense?
        Hide
        Hamed Madani added a comment -

        Made this patch late at night. Thanks for reviewing it Lars George. I can certainly bring back writeToWAL for backwards compatibility. Do we want to keep this backward compatibility with 0.95 and 0.98 ? or only 0.94 ?

        Also currently, writeToWal defaults to true when it's not set by user which will translate to "Durability.SYNC_WAL". If we were to keep writeToWAL I suggest we remove the default value, so we can set Durability only when writeToWal or TDurability is explicitly set and otherwise default to CF Durability setting. What do you think ?

        Show
        Hamed Madani added a comment - Made this patch late at night. Thanks for reviewing it Lars George . I can certainly bring back writeToWAL for backwards compatibility. Do we want to keep this backward compatibility with 0.95 and 0.98 ? or only 0.94 ? Also currently, writeToWal defaults to true when it's not set by user which will translate to "Durability.SYNC_WAL". If we were to keep writeToWAL I suggest we remove the default value, so we can set Durability only when writeToWal or TDurability is explicitly set and otherwise default to CF Durability setting. What do you think ?
        Hide
        Lars George added a comment -

        Good question on where to drop this, we would need to deprecate it first. I suggest we follow Mutation and setDurability(), which deprecated setWriteToWAL() in 0.94 and dropped it in 0.95 and trunk.

        Well, with the default of writeToWAL, I thought if we first check if isSetDurability() we favor it over the old setWriteToWAL, then default or not it is ignore. We should also set a default for TDurability though:

          /**
           * If this is for tables durability, use HBase's global default value (SYNC_WAL).
           * Otherwise, if this is for mutation, use the table's default setting to determine durability.
           * This must remain the first option.
           */
          USE_DEFAULT,
        

        This is from the Durability.java class. Can we somehow derive this on the Thrift side? I guess we can add a TDurability.USE_DEFAULT, make it the default and then pass it on?

        Show
        Lars George added a comment - Good question on where to drop this, we would need to deprecate it first. I suggest we follow Mutation and setDurability(), which deprecated setWriteToWAL() in 0.94 and dropped it in 0.95 and trunk. Well, with the default of writeToWAL, I thought if we first check if isSetDurability() we favor it over the old setWriteToWAL, then default or not it is ignore. We should also set a default for TDurability though: /** * If this is for tables durability, use HBase's global default value (SYNC_WAL). * Otherwise, if this is for mutation, use the table's default setting to determine durability. * This must remain the first option. */ USE_DEFAULT, This is from the Durability.java class. Can we somehow derive this on the Thrift side? I guess we can add a TDurability.USE_DEFAULT, make it the default and then pass it on?
        Hide
        Hamed Madani added a comment -

        Well I actually had a default value for TDurability, but then I saw if we don't set durability, it defaults to CF durability setting. I found it more efficient to not set the default on thrift side because client will be sending an extra field (default value) when thrift server doesn't need it. (Defaults is set without calling setDurability)

        So if you agree I can drop writeToWal for trunk patch and depreciate it for 0.94 ?

        Show
        Hamed Madani added a comment - Well I actually had a default value for TDurability, but then I saw if we don't set durability, it defaults to CF durability setting. I found it more efficient to not set the default on thrift side because client will be sending an extra field (default value) when thrift server doesn't need it. (Defaults is set without calling setDurability) So if you agree I can drop writeToWal for trunk patch and depreciate it for 0.94 ?
        Hide
        Lars George added a comment -

        Good thinking on not setting it to save an extra field. But what about the default of writeToWAL? Can we simply remove it and not break compatibility? Because if not, then we are always setting it to its default as opposed to defaulting to the CF dura settings?

        Agreed on the deprecation for 0.94 and drop in trunk.

        Show
        Lars George added a comment - Good thinking on not setting it to save an extra field. But what about the default of writeToWAL? Can we simply remove it and not break compatibility? Because if not, then we are always setting it to its default as opposed to defaulting to the CF dura settings? Agreed on the deprecation for 0.94 and drop in trunk.
        Hide
        Hamed Madani added a comment -

        For 0.94 we can drop the default for writeToWAL to avoid sending an extra field. I just double checked and for 0.94

          private boolean writeToWAL = true;
        

        writeToWAL get initialized to true , so I suggest we drop default value for writeToWAL, add Durability to Mutation (Apparently Increment doesn't support it since it doesn't extend Mutation in 0.94), then as you suggested we check for isSetDurability, if not set then isSetwriteToWAL and if not set then just don't set writeToWAL since it gets initialized to true inside Mutation class. What do you think ? Also do we need a test for this ? I wasn't sure how to test it.

        Show
        Hamed Madani added a comment - For 0.94 we can drop the default for writeToWAL to avoid sending an extra field. I just double checked and for 0.94 private boolean writeToWAL = true ; writeToWAL get initialized to true , so I suggest we drop default value for writeToWAL, add Durability to Mutation (Apparently Increment doesn't support it since it doesn't extend Mutation in 0.94), then as you suggested we check for isSetDurability, if not set then isSetwriteToWAL and if not set then just don't set writeToWAL since it gets initialized to true inside Mutation class. What do you think ? Also do we need a test for this ? I wasn't sure how to test it.
        Hide
        Lars George added a comment -

        If possible, you could somehow emulate the various settings and check if the resulting object reacts the way it should? Somehow we need to make sure that the above strategy works.

        Show
        Lars George added a comment - If possible, you could somehow emulate the various settings and check if the resulting object reacts the way it should? Somehow we need to make sure that the above strategy works.
        Hide
        Hamed Madani added a comment -

        First uploading the easier trunk patch. Added test.

        Show
        Hamed Madani added a comment - First uploading the easier trunk patch. Added test.
        Hide
        Hamed Madani added a comment -

        added 0.94, also test

        Show
        Hamed Madani added a comment - added 0.94, also test
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12593678/HBASE-8947-v3-0.94.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6437//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12593678/HBASE-8947-v3-0.94.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6437//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12593661/HBASE-8947-v3.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12593661/HBASE-8947-v3.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6436//console This message is automatically generated.
        Hide
        Lars George added a comment -

        Hey Hamed Madani, I think there is a flaw in the numbering of the Thrift fields:

        In trunk (and 0.95 then):

         struct TPut {
           1: required binary row,
           2: required list<TColumnValue> columnValues
           3: optional i64 timestamp,
        -  4: optional bool writeToWal = 1,
        +  4: optional TDurability durability,
           5: optional map<binary, binary> attributes
         }
        

        and in 0.94:

         struct TPut {
           1: required binary row,
           2: required list<TColumnValue> columnValues
           3: optional i64 timestamp,
        -  4: optional bool writeToWal = 1,
        -  5: optional map<binary, binary> attributes
        +  4: optional bool writeToWal,
        +  5: optional TDurability durability,
        +  6: optional map<binary, binary> attributes
         }
        

        The "attributes" are changing their numbers, which means a 0.94 client will not work with a trunk server. You need to keep the numbers unique and so that they never change. In other words, the 0.94 patch should be:

         struct TPut {
           1: required binary row,
           2: required list<TColumnValue> columnValues
           3: optional i64 timestamp,
        -  4: optional bool writeToWal = 1,
        -  5: optional map<binary, binary> attributes
        +  4: optional bool writeToWal,
        +  6: optional TDurability durability,
        +  5: optional map<binary, binary> attributes
         }
        

        note the flipped numbering to keep the recently added "attributes" on ID #5. The trunk patch then should include

         struct TPut {
           1: required binary row,
           2: required list<TColumnValue> columnValues
           3: optional i64 timestamp,
        -  4: optional bool writeToWal = 1,
        +  6: optional TDurability durability,
           5: optional map<binary, binary> attributes
         }
        

        i.e. removing ID #4 but keeping ID #5 and adding #6 as per the above.

        Makes sense?

        Show
        Lars George added a comment - Hey Hamed Madani , I think there is a flaw in the numbering of the Thrift fields: In trunk (and 0.95 then): struct TPut { 1: required binary row, 2: required list<TColumnValue> columnValues 3: optional i64 timestamp, - 4: optional bool writeToWal = 1, + 4: optional TDurability durability, 5: optional map<binary, binary> attributes } and in 0.94: struct TPut { 1: required binary row, 2: required list<TColumnValue> columnValues 3: optional i64 timestamp, - 4: optional bool writeToWal = 1, - 5: optional map<binary, binary> attributes + 4: optional bool writeToWal, + 5: optional TDurability durability, + 6: optional map<binary, binary> attributes } The "attributes" are changing their numbers, which means a 0.94 client will not work with a trunk server. You need to keep the numbers unique and so that they never change. In other words, the 0.94 patch should be: struct TPut { 1: required binary row, 2: required list<TColumnValue> columnValues 3: optional i64 timestamp, - 4: optional bool writeToWal = 1, - 5: optional map<binary, binary> attributes + 4: optional bool writeToWal, + 6: optional TDurability durability, + 5: optional map<binary, binary> attributes } note the flipped numbering to keep the recently added "attributes" on ID #5. The trunk patch then should include struct TPut { 1: required binary row, 2: required list<TColumnValue> columnValues 3: optional i64 timestamp, - 4: optional bool writeToWal = 1, + 6: optional TDurability durability, 5: optional map<binary, binary> attributes } i.e. removing ID #4 but keeping ID #5 and adding #6 as per the above. Makes sense?
        Hide
        Hamed Madani added a comment -

        Thank you Lars George for reviewing the patch.
        Since 0.94 uses thrift 0.8.0 and trunk uses thrift 0.9.0 I believe clients updating to 0.95 or above have to re-generate their thrift files anyways. As for numbering thrift fields, I was under the impression that the fields should be ordered and a missing field number is not allowed. (i.e can not have 6: optional TDurability durability then 5: optional map<binary, binary> attributes )

        Show
        Hamed Madani added a comment - Thank you Lars George for reviewing the patch. Since 0.94 uses thrift 0.8.0 and trunk uses thrift 0.9.0 I believe clients updating to 0.95 or above have to re-generate their thrift files anyways. As for numbering thrift fields, I was under the impression that the fields should be ordered and a missing field number is not allowed. (i.e can not have 6: optional TDurability durability then 5: optional map<binary, binary> attributes )
        Hide
        Lars George added a comment -

        Hi Hamed Madani, I am not talking about the client adopting the new features, they need to do that eventually. But for a rolling upgrade, you can have an older client talking to a newer server, or vice versa. See this for reference. Similar to ProtoBufs (see "Assigning Tags") the IDs are what is used during serialization and deserialization to match values to fields. If you change this, then you mix up assignment.

        As for ordering, if you look at the Thrift example, there is a test including a "Backwards" test, which assigns the IDs out of order. I had assumed this is arbitrary (i.e. the ordering) as long as you match IDs to fields and not change them across versions.

        And you should be able to have gaps, since you can that way retire older fields - the deserialization will simply ignore them. I just wish Thrift had a proper documentation.

        Show
        Lars George added a comment - Hi Hamed Madani , I am not talking about the client adopting the new features, they need to do that eventually. But for a rolling upgrade, you can have an older client talking to a newer server, or vice versa. See this for reference. Similar to ProtoBufs (see "Assigning Tags") the IDs are what is used during serialization and deserialization to match values to fields. If you change this, then you mix up assignment. As for ordering, if you look at the Thrift example, there is a test including a "Backwards" test, which assigns the IDs out of order. I had assumed this is arbitrary (i.e. the ordering) as long as you match IDs to fields and not change them across versions. And you should be able to have gaps, since you can that way retire older fields - the deserialization will simply ignore them. I just wish Thrift had a proper documentation.
        Hide
        Lars George added a comment -

        For reference from online material:

        Structs
        ...
        Field identifiers will be automatically assigned if omitted, though they are strongly encouraged for versioning reasons discussed later.
        ...
        Versioning
        ...
        Thrift is robust in the face of versioning and data definition changes. This is critical to enable staged rollouts of changes to deployed services.
        ...
        Field Identifiers
        ...
        To avoid conflicts between manually and automatically assigned identifiers, fields with identifiers omitted are assigned identifiers decrementing from -1, and the language only supports the manual assignment of positive identifiers.
        ...
        If a field identifier is not recognized, the generated code can use the type specifier to skip the unknown field without any error.

        So there is no notion of ordering, but states the fact that there may be gaps because of older to newer definitions.

        Show
        Lars George added a comment - For reference from online material: Structs ... Field identifiers will be automatically assigned if omitted, though they are strongly encouraged for versioning reasons discussed later. ... Versioning ... Thrift is robust in the face of versioning and data definition changes. This is critical to enable staged rollouts of changes to deployed services. ... Field Identifiers ... To avoid conflicts between manually and automatically assigned identifiers, fields with identifiers omitted are assigned identifiers decrementing from -1, and the language only supports the manual assignment of positive identifiers. ... If a field identifier is not recognized, the generated code can use the type specifier to skip the unknown field without any error. So there is no notion of ordering, but states the fact that there may be gaps because of older to newer definitions.
        Hide
        Hamed Madani added a comment -

        Thank you Lars George for the explanation. It does make sense now Updated the patches.

        Show
        Hamed Madani added a comment - Thank you Lars George for the explanation. It does make sense now Updated the patches.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12594132/HBASE-8947-v4-.094.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6461//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12594132/HBASE-8947-v4-.094.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6461//console This message is automatically generated.
        Hide
        Lars George added a comment -

        Hi Hamed Madani, did you by any chance miss the TIncrement changes in the 0.94 patch? Looking at what you attached, it differs from the trunk patch by ignoring the increments.

        Show
        Lars George added a comment - Hi Hamed Madani , did you by any chance miss the TIncrement changes in the 0.94 patch? Looking at what you attached, it differs from the trunk patch by ignoring the increments.
        Hide
        Hamed Madani added a comment -

        Yes Lars. Since Increment doesn't extend Mutation in 0.94, it doesn't support Durability.

        Show
        Hamed Madani added a comment - Yes Lars. Since Increment doesn't extend Mutation in 0.94, it doesn't support Durability.
        Hide
        Lars George added a comment -

        Ah right, you mentioned this earlier. Duh.

        Testing you patches now, but looking at them in the raw, they look pretty good already. Thanks again Hamed!

        Show
        Lars George added a comment - Ah right, you mentioned this earlier. Duh. Testing you patches now, but looking at them in the raw, they look pretty good already. Thanks again Hamed!
        Hide
        Lars George added a comment -

        All tests pass, committing to all major branches. Thanks Hamed!

        Show
        Lars George added a comment - All tests pass, committing to all major branches. Thanks Hamed!
        Hide
        Elliott Clark added a comment -

        Looks like this broke trunk compiles.

        [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:2.5.1:compile (default-compile) on project hbase-server: Compilation failure: Compilation failure:
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[68,9] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TPut
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[368,9] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TPut
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[376,28] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TPut
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[88,9] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TDelete
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[434,9] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TDelete
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[442,31] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TDelete
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[61,9] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TIncrement
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[327,9] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TIncrement
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[335,34] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TIncrement
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftUtilities.java:[413,49] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class ThriftUtilities
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[163,92] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TPut
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[434,23] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TPut
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[776,34] error: cannot find symbol
        [ERROR] symbol:   variable TDurability
        [ERROR] location: class TPutStandardScheme
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[933,28] error: cannot find symbol
        [ERROR] symbol:   variable TDurability
        [ERROR] location: class TPutTupleScheme
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[192,92] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TDelete
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[508,23] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TDelete
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[891,34] error: cannot find symbol
        [ERROR] symbol:   variable TDurability
        [ERROR] location: class TDeleteStandardScheme
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[1074,28] error: cannot find symbol
        [ERROR] symbol:   variable TDurability
        [ERROR] location: class TDeleteTupleScheme
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[149,92] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TIncrement
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[385,23] error: cannot find symbol
        [ERROR] symbol:   class TDurability
        [ERROR] location: class TIncrement
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[687,34] error: cannot find symbol
        [ERROR] symbol:   variable TDurability
        [ERROR] location: class TIncrementStandardScheme
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[829,28] error: cannot find symbol
        [ERROR] symbol:   variable TDurability
        [ERROR] location: class TIncrementTupleScheme
        [ERROR] /Users/eclark/Code/public/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftUtilities.java:[414,32] error: strings in switch are not supported in -source 1.6
        
        Show
        Elliott Clark added a comment - Looks like this broke trunk compiles. [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:2.5.1:compile ( default -compile) on project hbase-server: Compilation failure: Compilation failure: [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[68,9] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TPut [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[368,9] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TPut [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[376,28] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TPut [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[88,9] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TDelete [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[434,9] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TDelete [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[442,31] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TDelete [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[61,9] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TIncrement [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[327,9] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TIncrement [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[335,34] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TIncrement [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftUtilities.java:[413,49] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class ThriftUtilities [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[163,92] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TPut [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[434,23] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TPut [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[776,34] error: cannot find symbol [ERROR] symbol: variable TDurability [ERROR] location: class TPutStandardScheme [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TPut.java:[933,28] error: cannot find symbol [ERROR] symbol: variable TDurability [ERROR] location: class TPutTupleScheme [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[192,92] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TDelete [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[508,23] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TDelete [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[891,34] error: cannot find symbol [ERROR] symbol: variable TDurability [ERROR] location: class TDeleteStandardScheme [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TDelete.java:[1074,28] error: cannot find symbol [ERROR] symbol: variable TDurability [ERROR] location: class TDeleteTupleScheme [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[149,92] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TIncrement [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[385,23] error: cannot find symbol [ERROR] symbol: class TDurability [ERROR] location: class TIncrement [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[687,34] error: cannot find symbol [ERROR] symbol: variable TDurability [ERROR] location: class TIncrementStandardScheme [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/generated/TIncrement.java:[829,28] error: cannot find symbol [ERROR] symbol: variable TDurability [ERROR] location: class TIncrementTupleScheme [ERROR] /Users/eclark/Code/ public /hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/thrift2/ThriftUtilities.java:[414,32] error: strings in switch are not supported in -source 1.6
        Hide
        Andrew Purtell added a comment -

        I see the same breakage.

        Show
        Andrew Purtell added a comment - I see the same breakage.
        Hide
        Lars Hofhansl added a comment -

        Broke 0.94 build as well. Let's revert for now (or have an addendum soon).

        Show
        Lars Hofhansl added a comment - Broke 0.94 build as well. Let's revert for now (or have an addendum soon).
        Hide
        Lars Hofhansl added a comment -

        Lars G. probably just forgot to add TDurability.

        Show
        Lars Hofhansl added a comment - Lars G. probably just forgot to add TDurability.
        Hide
        Lars George added a comment -

        Yes I did, sorry. Coming up ASAP.

        Show
        Lars George added a comment - Yes I did, sorry. Coming up ASAP.
        Hide
        stack added a comment -

        Here is my revert of this on trunk and 0.94.

        Show
        stack added a comment - Here is my revert of this on trunk and 0.94.
        Hide
        Lars George added a comment -

        Added file.

        Show
        Lars George added a comment - Added file.
        Hide
        Lars Hofhansl added a comment -

        I just added TDurability locally to 0.94 and all is fine now. This is the part of SVN I hate (I have forgotten to add new file before commit).

        Show
        Lars Hofhansl added a comment - I just added TDurability locally to 0.94 and all is fine now. This is the part of SVN I hate (I have forgotten to add new file before commit).
        Hide
        stack added a comment -

        Lars George Sorry. Reverted from trunk. I will get out of your way now you are on it. Sorry about that. Let me know if you want me to reapply.

        Show
        stack added a comment - Lars George Sorry. Reverted from trunk. I will get out of your way now you are on it. Sorry about that. Let me know if you want me to reapply.
        Hide
        Lars George added a comment -

        Yeah, my bad for not double checking. I will reapply the revert ASAP.

        Show
        Lars George added a comment - Yeah, my bad for not double checking. I will reapply the revert ASAP.
        Hide
        Lars George added a comment -

        Added missing file, sorry guys! Applied patch to trunk again after Stack's revert.

        Show
        Lars George added a comment - Added missing file, sorry guys! Applied patch to trunk again after Stack's revert.
        Hide
        stack added a comment -

        Lars George no harm done

        Show
        stack added a comment - Lars George no harm done

          People

          • Assignee:
            Hamed Madani
            Reporter:
            Hamed Madani
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development