Details

    • Type: Task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.7.2, 1.8.0
    • Component/s: None
    • Labels:
      None

      Description

      Commons VFS 2.1 is nearing release. When released we need to remove the VFS related classes in the start module, update the imports, and update the version in the pom. Will set fixVersions when VFS is released.

        Issue Links

          Activity

          Hide
          dlmarion Dave Marion added a comment -

          I removed ReadOnlyHdfsFileProviderTest in 1.7 and beyond. I think my work is done here.

          Show
          dlmarion Dave Marion added a comment - I removed ReadOnlyHdfsFileProviderTest in 1.7 and beyond. I think my work is done here.
          Hide
          dlmarion Dave Marion added a comment -

          Ok, I reverted the commit for updating Commons VFS from 2.0 to 2.1 in the Accumulo 1.6 branch. I merged that change up to 1.7, reverted the revert commit, and merged that all the way up to master.

          Show
          dlmarion Dave Marion added a comment - Ok, I reverted the commit for updating Commons VFS from 2.0 to 2.1 in the Accumulo 1.6 branch. I merged that change up to 1.7, reverted the revert commit, and merged that all the way up to master.
          Hide
          dlmarion Dave Marion added a comment -

          So the tests pass with the hadoop 2 profile, but not the hadoop 1 profile. The commons vfs2 HDFS dependency is Hadoop 2.6.0. It appears that the hadoop-1 profile was removed in Accumulo 1.7.0. What's the appropriate action here? Revert the commit in the Accumulo 1.6 branch so that it continues to work? I think the ReadOnlyHdfsFileProviderTest class can be removed in Accumulo 1.7.0 and beyond.

          Show
          dlmarion Dave Marion added a comment - So the tests pass with the hadoop 2 profile, but not the hadoop 1 profile. The commons vfs2 HDFS dependency is Hadoop 2.6.0. It appears that the hadoop-1 profile was removed in Accumulo 1.7.0. What's the appropriate action here? Revert the commit in the Accumulo 1.6 branch so that it continues to work? I think the ReadOnlyHdfsFileProviderTest class can be removed in Accumulo 1.7.0 and beyond.
          Hide
          dlmarion Dave Marion added a comment -

          I think they were duplicative, but I will check.

          Show
          dlmarion Dave Marion added a comment - I think they were duplicative, but I will check.
          Hide
          ctubbsii Christopher Tubbs added a comment -

          Okay, no rush. But also, not just removed, but if they are useful tests, they should be in commons-vfs. I didn't see them when I looked in the source code, but I may have overlooked them.

          Show
          ctubbsii Christopher Tubbs added a comment - Okay, no rush. But also, not just removed, but if they are useful tests, they should be in commons-vfs. I didn't see them when I looked in the source code, but I may have overlooked them.
          Hide
          dlmarion Dave Marion added a comment -

          They were missed, they should be removed. My IDE didn't complain about invalid references and I missed them. I will do it when I get back on Monday.

          Show
          dlmarion Dave Marion added a comment - They were missed, they should be removed. My IDE didn't complain about invalid references and I missed them. I will do it when I get back on Monday.
          Hide
          ctubbsii Christopher Tubbs added a comment -

          There are two remaining test classes that don't seem to have anything to do with Accumulo left in the accumulo-start module after this change. They seem to only be testing features of vfs hdfs support. Shouldn't those be moved over to be tests within commons-vfs instead?

          These two are VfsClassLoaderTest and ReadOnlyHdfsFileProviderTest.

          Show
          ctubbsii Christopher Tubbs added a comment - There are two remaining test classes that don't seem to have anything to do with Accumulo left in the accumulo-start module after this change. They seem to only be testing features of vfs hdfs support. Shouldn't those be moved over to be tests within commons-vfs instead? These two are VfsClassLoaderTest and ReadOnlyHdfsFileProviderTest.
          Hide
          elserj Josh Elser added a comment -

          The VFS 2.1 release notes state that it is source and binary compatible

          If it says it's source compatible, then I goofed the announcement. I don't think it is source compatible with 2.0.

          It is binary compatible though which should be enough for our uses.

          Show
          elserj Josh Elser added a comment - The VFS 2.1 release notes state that it is source and binary compatible If it says it's source compatible, then I goofed the announcement. I don't think it is source compatible with 2.0. It is binary compatible though which should be enough for our uses.
          Hide
          busbey Sean Busbey added a comment -

          oh. Maybe they fixed the issues then? The last I had seen on an RC was that there were some source compatibility issues that folks judged low-risk for downstream.

          Presuming that's the case, a note that just says we updated the version would be fine.

          Show
          busbey Sean Busbey added a comment - oh. Maybe they fixed the issues then? The last I had seen on an RC was that there were some source compatibility issues that folks judged low-risk for downstream. Presuming that's the case, a note that just says we updated the version would be fine.
          Hide
          dlmarion Dave Marion added a comment -

          Bumped vfs2 version 2.1, updated imports, removed VFS related code, built with all versions with 'mvn clean test'

          Show
          dlmarion Dave Marion added a comment - Bumped vfs2 version 2.1, updated imports, removed VFS related code, built with all versions with 'mvn clean test'
          Hide
          dlmarion Dave Marion added a comment -

          Sean Busbey what breakage? The announcement that just came out for VFS 2.1 says its binary compatible. The VFS 2.1 release notes state that it is source and binary compatible.

          Show
          dlmarion Dave Marion added a comment - Sean Busbey what breakage? The announcement that just came out for VFS 2.1 says its binary compatible. The VFS 2.1 release notes state that it is source and binary compatible.
          Hide
          busbey Sean Busbey added a comment -

          that's what I figured. Dave Marion can we make sure to have a release note covering this change that points to the release notes for the VFS release (presuming that includes some docs on the breakage)?

          Show
          busbey Sean Busbey added a comment - that's what I figured. Dave Marion can we make sure to have a release note covering this change that points to the release notes for the VFS release (presuming that includes some docs on the breakage)?
          Hide
          dlmarion Dave Marion added a comment -

          I have been using 2.1-SNAPSHOT for a while now with no issues.

          Show
          dlmarion Dave Marion added a comment - I have been using 2.1-SNAPSHOT for a while now with no issues.
          Hide
          elserj Josh Elser added a comment -

          Or do the known fixed issues outweigh the risk to downstream folks impacted by our classpath?

          IMO, it's unusable presently, so yeah, I don't think we should be worried about compat (and the final scope of backwards compatibility that was broken was pretty small). This is likely also brutally obvious given my previous chatter on dev@c.a.o

          Show
          elserj Josh Elser added a comment - Or do the known fixed issues outweigh the risk to downstream folks impacted by our classpath? IMO, it's unusable presently, so yeah, I don't think we should be worried about compat (and the final scope of backwards compatibility that was broken was pretty small). This is likely also brutally obvious given my previous chatter on dev@c.a.o
          Hide
          busbey Sean Busbey added a comment -

          (I'm guessing based on the long history around VFS that the answer to that last questions is yes?)

          Show
          busbey Sean Busbey added a comment - (I'm guessing based on the long history around VFS that the answer to that last questions is yes?)
          Hide
          busbey Sean Busbey added a comment -

          re: inclusion in 1.6.z or 1.7.z maintenance releases, do we care about any of the compatibility things flagged in the 2.1 release? Or do the known fixed issues outweigh the risk to downstream folks impacted by our classpath?

          Show
          busbey Sean Busbey added a comment - re: inclusion in 1.6.z or 1.7.z maintenance releases, do we care about any of the compatibility things flagged in the 2.1 release? Or do the known fixed issues outweigh the risk to downstream folks impacted by our classpath?
          Hide
          elserj Josh Elser added a comment -

          Also, shouldn't this also be tagged for 1.6 and 1.7?

          Show
          elserj Josh Elser added a comment - Also, shouldn't this also be tagged for 1.6 and 1.7?
          Hide
          elserj Josh Elser added a comment -

          VFS 2.1 has been released. I hope to get to this soon.

          Careful... it has not been officially released (PMC has not sent out a notice yet), but there are voted-upon JARs up in Nexus yes...

          Show
          elserj Josh Elser added a comment - VFS 2.1 has been released. I hope to get to this soon. Careful... it has not been officially released (PMC has not sent out a notice yet), but there are voted-upon JARs up in Nexus yes...
          Hide
          dlmarion Dave Marion added a comment -

          VFS 2.1 has been released. I hope to get to this soon.

          Show
          dlmarion Dave Marion added a comment - VFS 2.1 has been released. I hope to get to this soon.
          Hide
          elserj Josh Elser added a comment -

          We have a bunch of sleep statements in related VFS tests in AccumuloReloadingVFSClassLoaderTest and VfsClassLoaderTest which were added to make them reliably pass. A quick test would be to remove those sleeps and ensure that the tests continue to pass repeatedly.

          Show
          elserj Josh Elser added a comment - We have a bunch of sleep statements in related VFS tests in AccumuloReloadingVFSClassLoaderTest and VfsClassLoaderTest which were added to make them reliably pass. A quick test would be to remove those sleeps and ensure that the tests continue to pass repeatedly.
          Hide
          dlmarion Dave Marion added a comment -

          VFS-487 has been fixed in VFS 2.1, so this related issue should be tested.

          Show
          dlmarion Dave Marion added a comment - VFS-487 has been fixed in VFS 2.1, so this related issue should be tested.

            People

            • Assignee:
              dlmarion Dave Marion
              Reporter:
              dlmarion Dave Marion
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0h
                0h
                Logged:
                Time Spent - 2h 20m
                2h 20m

                  Development