Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.6.0
    • Component/s: None
    • Labels:
      None

      Description

      While examining API changes for 1.6.0 I noticed some non-deprecated methods were removed. I am not sure how important these are, but technically these methods are in the public API. Opening this issue to document what I found.

      I compared 1.6.0 to 1.5.0.

      In ACCUMULO-1674 the following methods were removed

      package org.apache.accumulo.core.client.mapreduce.lib.util
      ConfiguratorBase.getToken ( Class<?>, Configuration ) [static]  :  byte[ ]
      ConfiguratorBase.getTokenClass ( Class<?> ,Configuration) [static]  :  String
      

      In ACCUMULO-391 the following method was removed

      package org.apache.accumulo.core.client.mapreduce.lib.util
      InputConfigurator.getTabletLocator ( Class<?>, Configuration ) [static]  : TabletLocator 
      

      In ACCUMULO-391 the following method was removed and not properly fixed in ACCUMULO-2586

      accumulo-core.jar, RangeInputSplit.class
      package org.apache.accumulo.core.client.mapred
      InputFormatBase.RangeInputSplit.InputFormatBase.RangeInputSplit ( String table, Range range, String[ ] locations )
      package org.apache.accumulo.core.client.mapreduce
      InputFormatBase.RangeInputSplit.InputFormatBase.RangeInputSplit ( String table, Range range, String[ ] locations ) 
      

      It seems like the following were removed in ACCUMULO-1854

      package org.apache.accumulo.core.client.mapred
      InputFormatBase.RecordReaderBase<K.setupIterators (JobConf job, Scanner scanner )  :  void
      package org.apache.accumulo.core.client.mapreduce
      InputFormatBase.RecordReaderBase<K.setupIterators (TaskAttemptContext context, Scanner scanner)  :  void
      

      In ACCUMULO-1018 the following method was removed

      package org.apache.accumulo.core.client
      MutationsRejectedException.MutationsRejectedException ( List, HashMap, Set, Collection, int cause, Throwable cvsList ) 
      

        Issue Links

          Activity

          Hide
          Josh Elser added a comment -

          RangeInputSplit problems should have been mitigated in ACCUMULO-2586. Verification is welcome.

          Show
          Josh Elser added a comment - RangeInputSplit problems should have been mitigated in ACCUMULO-2586 . Verification is welcome.
          Hide
          Mike Drob added a comment -

          We need to take the most permissive interpretation of the 1.5.0 README's statements about API compatibility possible, and treat methods accordingly.

          Show
          Mike Drob added a comment - We need to take the most permissive interpretation of the 1.5.0 README's statements about API compatibility possible, and treat methods accordingly.
          Hide
          Christopher Tubbs added a comment -

          Looking at this again, I cannot imagine any sensible way to reintroduce the removed ConfiguratorBase.getToken(...), because its ONLY value was to reconstruct an AuthenticationToken with CredentialHelper, which was never public API. This is a bit inane, because the methods that replace it give you an AuthenticationToken directly, and CredentialHelper was removed.

          Further, we just don't store the bytes that way anymore, so I can give a byte array back, representing a serialized form of a token, but it won't be what any caller would expect... unless I reintroduce CredentialHelper's serialization mechanism, only to serialize an AuthenticationToken to bytes, so that a caller can reconstruct an AuthenticationToken from those bytes... which they could only previously do with CredentialHelper, which no longer exists, and all this is moot, because the replacement methods give you an AuthenticationToken directly in the first place.

          Any fix would be a hack to reintroduce a method that is completely useless, and whose usage would depend on something that was never in the public API (CredentialHelper).

          What I can do, is rename the ConfiguratorBase package to "impl" instead of "util", and replace it with a mostly 1.5.x-compatible implementation (either without a getToken() method or one which throws an exception).

          Show
          Christopher Tubbs added a comment - Looking at this again, I cannot imagine any sensible way to reintroduce the removed ConfiguratorBase.getToken(...), because its ONLY value was to reconstruct an AuthenticationToken with CredentialHelper, which was never public API. This is a bit inane, because the methods that replace it give you an AuthenticationToken directly, and CredentialHelper was removed. Further, we just don't store the bytes that way anymore, so I can give a byte array back, representing a serialized form of a token, but it won't be what any caller would expect... unless I reintroduce CredentialHelper's serialization mechanism, only to serialize an AuthenticationToken to bytes, so that a caller can reconstruct an AuthenticationToken from those bytes... which they could only previously do with CredentialHelper, which no longer exists, and all this is moot, because the replacement methods give you an AuthenticationToken directly in the first place. Any fix would be a hack to reintroduce a method that is completely useless, and whose usage would depend on something that was never in the public API (CredentialHelper). What I can do, is rename the ConfiguratorBase package to "impl" instead of "util", and replace it with a mostly 1.5.x-compatible implementation (either without a getToken() method or one which throws an exception).
          Hide
          Mike Drob added a comment -

          Copying from IRC:

          The bytes can be given to AuthenticationToken.AuthenticationTokenSerializer.deserialize() which is almost spot on to what CredentialHelper did, but a little better about error handling. I'm not going to go as far as suggesting that we reintroduce CredentialHelper as deprecated and pointing to AT.ATS.deserialize() but that is certainly one option. At the very least, use the javadoc to mark it deprecated, and point users to both the methods that return a token directly and the new deserialize method.

          Show
          Mike Drob added a comment - Copying from IRC: The bytes can be given to AuthenticationToken.AuthenticationTokenSerializer.deserialize() which is almost spot on to what CredentialHelper did, but a little better about error handling. I'm not going to go as far as suggesting that we reintroduce CredentialHelper as deprecated and pointing to AT.ATS.deserialize() but that is certainly one option. At the very least, use the javadoc to mark it deprecated, and point users to both the methods that return a token directly and the new deserialize method.
          Hide
          ASF subversion and git services added a comment -

          Commit 382bfdcef9f34685724b3d9614f0df7c1cda26b8 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from Christopher Tubbs
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=382bfdc ]

          ACCUMULO-2659 Properly deprecate public API changes to mapred utils

          Show
          ASF subversion and git services added a comment - Commit 382bfdcef9f34685724b3d9614f0df7c1cda26b8 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from Christopher Tubbs [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=382bfdc ] ACCUMULO-2659 Properly deprecate public API changes to mapred utils
          Hide
          ASF subversion and git services added a comment -

          Commit f51d7bba06e3b3403d73a2c69962613e486da362 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from Christopher Tubbs
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f51d7bb ]

          ACCUMULO-2171 Update CHANGES

          Include ACCUMULO-1486, ACCUMULO-2482, ACCUMULO-2659, ACCUMULO-2660

          Show
          ASF subversion and git services added a comment - Commit f51d7bba06e3b3403d73a2c69962613e486da362 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from Christopher Tubbs [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f51d7bb ] ACCUMULO-2171 Update CHANGES Include ACCUMULO-1486 , ACCUMULO-2482 , ACCUMULO-2659 , ACCUMULO-2660
          Hide
          Bill Havanki added a comment -

          Was any testing done on these changes, maybe some MR jobs? Thanks.

          Show
          Bill Havanki added a comment - Was any testing done on these changes, maybe some MR jobs? Thanks.
          Hide
          Josh Elser added a comment -

          I just ran some pig jobs over an in progress state of these changes. Not affected.

          Show
          Josh Elser added a comment - I just ran some pig jobs over an in progress state of these changes. Not affected.
          Hide
          ASF subversion and git services added a comment -

          Commit 882cfc881f0e3d6fa504761bf456ebb6f3f61365 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from [~keith_turner]
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=882cfc8 ]

          ACCUMULO-2659 added MRE constructor back and deprecated it

          Show
          ASF subversion and git services added a comment - Commit 882cfc881f0e3d6fa504761bf456ebb6f3f61365 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from [~keith_turner] [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=882cfc8 ] ACCUMULO-2659 added MRE constructor back and deprecated it
          Hide
          ASF subversion and git services added a comment -

          Commit 382bfdcef9f34685724b3d9614f0df7c1cda26b8 in accumulo's branch refs/heads/master from Christopher Tubbs
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=382bfdc ]

          ACCUMULO-2659 Properly deprecate public API changes to mapred utils

          Show
          ASF subversion and git services added a comment - Commit 382bfdcef9f34685724b3d9614f0df7c1cda26b8 in accumulo's branch refs/heads/master from Christopher Tubbs [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=382bfdc ] ACCUMULO-2659 Properly deprecate public API changes to mapred utils
          Hide
          ASF subversion and git services added a comment -

          Commit f51d7bba06e3b3403d73a2c69962613e486da362 in accumulo's branch refs/heads/master from Christopher Tubbs
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f51d7bb ]

          ACCUMULO-2171 Update CHANGES

          Include ACCUMULO-1486, ACCUMULO-2482, ACCUMULO-2659, ACCUMULO-2660

          Show
          ASF subversion and git services added a comment - Commit f51d7bba06e3b3403d73a2c69962613e486da362 in accumulo's branch refs/heads/master from Christopher Tubbs [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f51d7bb ] ACCUMULO-2171 Update CHANGES Include ACCUMULO-1486 , ACCUMULO-2482 , ACCUMULO-2659 , ACCUMULO-2660
          Hide
          ASF subversion and git services added a comment -

          Commit 882cfc881f0e3d6fa504761bf456ebb6f3f61365 in accumulo's branch refs/heads/master from [~keith_turner]
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=882cfc8 ]

          ACCUMULO-2659 added MRE constructor back and deprecated it

          Show
          ASF subversion and git services added a comment - Commit 882cfc881f0e3d6fa504761bf456ebb6f3f61365 in accumulo's branch refs/heads/master from [~keith_turner] [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=882cfc8 ] ACCUMULO-2659 added MRE constructor back and deprecated it
          Hide
          ASF subversion and git services added a comment -

          Commit 1586778 from kturner@apache.org in branch 'site/trunk'
          [ https://svn.apache.org/r1586778 ]

          ACCUMULO-2396 updated release notes now that ACCUMULO-2659 is fixed

          Show
          ASF subversion and git services added a comment - Commit 1586778 from kturner@apache.org in branch 'site/trunk' [ https://svn.apache.org/r1586778 ] ACCUMULO-2396 updated release notes now that ACCUMULO-2659 is fixed
          Hide
          ASF subversion and git services added a comment -

          Commit 019edb1614008485f523deecaef88c7455e1e404 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from Josh Elser
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=019edb1 ]

          ACCUMULO-2659 Fall back to the configuration when the iterator setting aren't in the split

          Show
          ASF subversion and git services added a comment - Commit 019edb1614008485f523deecaef88c7455e1e404 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from Josh Elser [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=019edb1 ] ACCUMULO-2659 Fall back to the configuration when the iterator setting aren't in the split
          Hide
          ASF subversion and git services added a comment -

          Commit 019edb1614008485f523deecaef88c7455e1e404 in accumulo's branch refs/heads/master from Josh Elser
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=019edb1 ]

          ACCUMULO-2659 Fall back to the configuration when the iterator setting aren't in the split

          Show
          ASF subversion and git services added a comment - Commit 019edb1614008485f523deecaef88c7455e1e404 in accumulo's branch refs/heads/master from Josh Elser [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=019edb1 ] ACCUMULO-2659 Fall back to the configuration when the iterator setting aren't in the split
          Hide
          Josh Elser added a comment -

          Best as I can tell, this ticket also removed deprecated mapred(uce) classes during a merge commit. This is problematic because it was very unclear as to the commit which actually made the changes, and I can't cleanly revert part or all of the changes of the deprecation removals without visual inspection.

          Because it changes things for 1.7.0, I've updated the fixVersion.

          Show
          Josh Elser added a comment - Best as I can tell, this ticket also removed deprecated mapred(uce) classes during a merge commit. This is problematic because it was very unclear as to the commit which actually made the changes, and I can't cleanly revert part or all of the changes of the deprecation removals without visual inspection. Because it changes things for 1.7.0, I've updated the fixVersion.
          Hide
          Christopher Tubbs added a comment -

          So, I took at a look at the merge commit in question. What it looks like happened is that these methods had already been dropped in a previous commit, after we had branched 1.6.0 from master to prepare for release. So, that drop was merged forward to master. This issue (ACCUMULO-2659) was intended to re-introduce them to 1.6.0 in a deprecated state, so that we'd have a smooth transition. Merging the commit to master, which re-added these methods, necessarily dropped them (effectively, a "git merge -s ours 1.6.0-SNAPSHOT" in master, although I doubt that command was precisely what was used.). So, it only looks like the merge dropped them, but really, the merge simply didn't re-add them to master, where they already had been dropped.

          Also, fixes to 1.6.0 are assumed to be also fixed in future versions, so 1.7.0 fixVersion isn't needed here. If something regressed in 1.7.0, or we have additional goals to keep these in for 1.7.0, that should go in a separate issue for 1.7.0 specifically.

          Show
          Christopher Tubbs added a comment - So, I took at a look at the merge commit in question. What it looks like happened is that these methods had already been dropped in a previous commit, after we had branched 1.6.0 from master to prepare for release. So, that drop was merged forward to master. This issue ( ACCUMULO-2659 ) was intended to re-introduce them to 1.6.0 in a deprecated state, so that we'd have a smooth transition. Merging the commit to master, which re-added these methods, necessarily dropped them (effectively, a "git merge -s ours 1.6.0-SNAPSHOT" in master, although I doubt that command was precisely what was used.). So, it only looks like the merge dropped them, but really, the merge simply didn't re-add them to master, where they already had been dropped. Also, fixes to 1.6.0 are assumed to be also fixed in future versions, so 1.7.0 fixVersion isn't needed here. If something regressed in 1.7.0, or we have additional goals to keep these in for 1.7.0, that should go in a separate issue for 1.7.0 specifically.

            People

            • Assignee:
              Christopher Tubbs
              Reporter:
              Keith Turner
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development