Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.0
    • Component/s: core
    • Labels:

      Description

      From parent issue:

      3. In several places including PreferredVolumeChooser, PerTableVolumeChooser and VolumeManagerImpl, the failsafe chooser is the RandomVolumeChooser which will include the instance volume that needs to be excluded. It would be useful to have a configurable failsafe in this situation.

        Issue Links

          Activity

          Hide
          ctubbsii Christopher Tubbs added a comment -

          I'm not so concerned about the PreferredVolumeChooser, because it's just one possible implementation, which could be swapped out with a different behavior. I'm slightly concerned about the PerTableVolumeChooser falling back to a default. It should probably fall back to whatever the default value is for table.volume.chooser (in DefaultConfiguration).

          I'm more concerned about other places, like whatever VolumeManagerImpl is doing, where we just don't have an appropriate scope available to configure for that case. In these cases, if the situation warrants it, I think we should think of it as a distinct scope, and do whatever it is we do for ACCUMULO-4085.

          There may be some utility in specifying a failsafe chooser (in the case of an actual failure... not just an overlooked scope which doesn't have a configuration point), for example if the specified one for that scope cannot be loaded from the classpath. However, at the risk of introducing an infinite regress (what happens if the failsafe fails to load?), it might be better to just fail until the config/classpath is fixed.

          Show
          ctubbsii Christopher Tubbs added a comment - I'm not so concerned about the PreferredVolumeChooser , because it's just one possible implementation, which could be swapped out with a different behavior. I'm slightly concerned about the PerTableVolumeChooser falling back to a default. It should probably fall back to whatever the default value is for table.volume.chooser (in DefaultConfiguration ). I'm more concerned about other places, like whatever VolumeManagerImpl is doing, where we just don't have an appropriate scope available to configure for that case. In these cases, if the situation warrants it, I think we should think of it as a distinct scope, and do whatever it is we do for ACCUMULO-4085 . There may be some utility in specifying a failsafe chooser (in the case of an actual failure... not just an overlooked scope which doesn't have a configuration point), for example if the specified one for that scope cannot be loaded from the classpath. However, at the risk of introducing an infinite regress (what happens if the failsafe fails to load?), it might be better to just fail until the config/classpath is fixed.
          Hide
          ctubbsii Christopher Tubbs added a comment -

          Discussing with Matt Peterson, it's probably better to have PerTableVolumeChooser fall back to the default table.volume.chooser in the system-wide configuration (accumulo-site.xml + system properties in zookeeper), not the one in the DefaultConfiguration.

          Show
          ctubbsii Christopher Tubbs added a comment - Discussing with Matt Peterson , it's probably better to have PerTableVolumeChooser fall back to the default table.volume.chooser in the system-wide configuration (accumulo-site.xml + system properties in zookeeper), not the one in the DefaultConfiguration .
          Hide
          Matt Peterson Matt Peterson added a comment -

          I'm still concerned about the use case where the PerTableVolumeChooser is used and a table is configured to use the PreferredVolumeChooser.

          In the case of a misconfiguration for the preferred volumes for that table, it doesn't seem that defaulting to the random volume chooser is appropriate. In this use case, it would make more sense to default to the site-wide table.volume.chooser.

          It also seems wrong for the PreferredVolumeChooser to fall back to the site-side table.volume.chooser which should be known only by the PerTableVolumeChooser.

          I'm proposing to have the site-wide fallback volume chooser not be specific to the PerTableVolumeChooser and to instead have a separate property that could be used for all volume choosers.

          Show
          Matt Peterson Matt Peterson added a comment - I'm still concerned about the use case where the PerTableVolumeChooser is used and a table is configured to use the PreferredVolumeChooser. In the case of a misconfiguration for the preferred volumes for that table, it doesn't seem that defaulting to the random volume chooser is appropriate. In this use case, it would make more sense to default to the site-wide table.volume.chooser. It also seems wrong for the PreferredVolumeChooser to fall back to the site-side table.volume.chooser which should be known only by the PerTableVolumeChooser. I'm proposing to have the site-wide fallback volume chooser not be specific to the PerTableVolumeChooser and to instead have a separate property that could be used for all volume choosers.
          Hide
          Matt Peterson Matt Peterson added a comment -

          Another option, which Christopher and I discussed, is for the PreferredVolumeChooser to not fallback at all but to instead throw an exception if it encounters a misconfiguration or other error. Then the caller can handle this appropriately, whether that caller is the PerTableVolumeChooser or VolumeManagerImpl.

          Show
          Matt Peterson Matt Peterson added a comment - Another option, which Christopher and I discussed, is for the PreferredVolumeChooser to not fallback at all but to instead throw an exception if it encounters a misconfiguration or other error. Then the caller can handle this appropriately, whether that caller is the PerTableVolumeChooser or VolumeManagerImpl.
          Hide
          ctubbsii Christopher Tubbs added a comment -

          I'm acting as proxy for Matt Peterson, to make his patches for (ACCUMULO-3384, ACCUMULO-4084, ACCUMULO-4085, ACCUMULO-4086, ACCUMULO-4087, and ACCUMULO-4523), available to the upstream project as pull requests and/or commits. Credit goes to him for producing the original patches, and to Mike Walch for rebase'ing it and resolving conflicts to master.

          Show
          ctubbsii Christopher Tubbs added a comment - I'm acting as proxy for Matt Peterson , to make his patches for ( ACCUMULO-3384 , ACCUMULO-4084 , ACCUMULO-4085 , ACCUMULO-4086 , ACCUMULO-4087 , and ACCUMULO-4523 ), available to the upstream project as pull requests and/or commits. Credit goes to him for producing the original patches, and to Mike Walch for rebase'ing it and resolving conflicts to master.
          Hide
          ivan.bella Ivan Bella added a comment -

          Are we ready to merge this one in?

          Show
          ivan.bella Ivan Bella added a comment - Are we ready to merge this one in?
          Hide
          ivan.bella Ivan Bella added a comment -

          I am needing this change for deployment onto our test system. Can we attempt to get this one wrapped up? I see there are now conflicts that have crept in. I will attempt to resolve those with another pull request into your branch.

          Show
          ivan.bella Ivan Bella added a comment - I am needing this change for deployment onto our test system. Can we attempt to get this one wrapped up? I see there are now conflicts that have crept in. I will attempt to resolve those with another pull request into your branch.
          Hide
          ctubbsii Christopher Tubbs added a comment -

          Yes, I'd like to close this one out also. I would like to take another look at it tomorrow before merging myself, but if you want to wrap it up before then, feel free to do that.

          Show
          ctubbsii Christopher Tubbs added a comment - Yes, I'd like to close this one out also. I would like to take another look at it tomorrow before merging myself, but if you want to wrap it up before then, feel free to do that.
          Hide
          ctubbsii Christopher Tubbs added a comment -

          I've squashed all the changes from all the contributors, rebase'd onto master, and updated the PR. I'd like to take one more look before the final merge, though. There's some things I'm not quite sure about that I want to review again. Additional eyes would also be appreciated.

          Show
          ctubbsii Christopher Tubbs added a comment - I've squashed all the changes from all the contributors, rebase'd onto master, and updated the PR. I'd like to take one more look before the final merge, though. There's some things I'm not quite sure about that I want to review again. Additional eyes would also be appreciated.

            People

            • Assignee:
              ctubbsii Christopher Tubbs
              Reporter:
              ctubbsii Christopher Tubbs
            • Votes:
              0 Vote for this issue
              Watchers:
              3 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 - 11h 50m
                11h 50m

                  Development