Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.8.0
    • Component/s: client
    • Labels:

      Description

      BinaryFormatter is currently used in a couple places in the shell, but the code is hard to read and understand. There is a static getlength, which is actually a setter, and all the instance calls end up going through unnecessary static methods.

      This combination makes it hard to reuse BinaryFormatter objects, or even use multiple, since the static state is likely to conflict.

        Issue Links

          Activity

          Hide
          Steve Blackmore added a comment -

          How important is it to maintain the static state, from what I can see the only time this value will make a difference is in ScanCommand.java the other use in GetSplitsCommand.java will always result in the showLength being the same as the Text.getLenght() it is appending.

          Show
          Steve Blackmore added a comment - How important is it to maintain the static state, from what I can see the only time this value will make a difference is in ScanCommand.java the other use in GetSplitsCommand.java will always result in the showLength being the same as the Text.getLenght() it is appending.
          Hide
          Eric Newton added a comment -

          Ideally, formatters would be stateless, except for some constant configuration.

          I don't know what's up with that funky getlength method.

          Show
          Eric Newton added a comment - Ideally, formatters would be stateless, except for some constant configuration. I don't know what's up with that funky getlength method.
          Hide
          Josh Elser added a comment -

          Hi Steve Blackmore, do you still intend to work on this?

          Show
          Josh Elser added a comment - Hi Steve Blackmore , do you still intend to work on this?
          Hide
          Matt Dailey added a comment -

          I started digging into this, and it looks more complicated than I hoped. It seems the purpose of getlength (which probably was meant to be named setlength) is really to support the -f option in ScanCommand. If that shell option is present, the code path starting here will set that static showLength value, and always use the BinaryFormatter rather than the set formatter (which could be considered a separate issue, too).

          It seems getlength changes the static state to get around the fact that the static FormatterFactory.getFormatter does not have the ability to set the show length (used here).

          I don't see a quick and easy fix here; maybe a change needs to happen in FormatterFactory to support additional parameters to send to the Formatter?

          Show
          Matt Dailey added a comment - I started digging into this, and it looks more complicated than I hoped. It seems the purpose of getlength (which probably was meant to be named setlength) is really to support the -f option in ScanCommand. If that shell option is present, the code path starting here will set that static showLength value, and always use the BinaryFormatter rather than the set formatter (which could be considered a separate issue, too). It seems getlength changes the static state to get around the fact that the static FormatterFactory.getFormatter does not have the ability to set the show length (used here ). I don't see a quick and easy fix here; maybe a change needs to happen in FormatterFactory to support additional parameters to send to the Formatter?
          Hide
          Josh Elser added a comment -

          Sounds reasonable to me. I don't think the Formatters fall into the public API, so you have the freedom to make changes as you see appropriate

          Show
          Josh Elser added a comment - Sounds reasonable to me. I don't think the Formatters fall into the public API, so you have the freedom to make changes as you see appropriate
          Hide
          Matt Dailey added a comment -

          Since adding an int maxLength option to Formatter.initialize would cause this issue to be brought up again whenever someone wants another configuration option down the road, I was thinking of changing Formatter.intialize to accept a hadoop Configuration object and let Formatters configure themselves.

          I like Configuration because of the helper getters and setters it has, but is that too heavyweight a solution for configuring a Formatter? The core package already brings in hadoop-client, so there's no additional dependency there.

          Show
          Matt Dailey added a comment - Since adding an int maxLength option to Formatter.initialize would cause this issue to be brought up again whenever someone wants another configuration option down the road, I was thinking of changing Formatter.intialize to accept a hadoop Configuration object and let Formatters configure themselves. I like Configuration because of the helper getters and setters it has, but is that too heavyweight a solution for configuring a Formatter? The core package already brings in hadoop-client , so there's no additional dependency there.
          Hide
          Josh Elser added a comment -

          Since adding an int maxLength option to Formatter.initialize would cause this issue to be brought up again whenever someone wants another configuration option down the road, I was thinking of changing Formatter.intialize to accept a hadoop Configuration object and let Formatters configure themselves.

          Good thinking. Convention-wise, we tend to make our own configuration objects instead of leveraging Hadoop Configuration objects. Examples include BatchWriterConfig, ClientConfiguration, ConditionalWriterConfig. I'd tend to go that route just for some consistency with the rest of the public API (assuming Formatters will eventually get pushed into the public API). WDYT?

          Show
          Josh Elser added a comment - Since adding an int maxLength option to Formatter.initialize would cause this issue to be brought up again whenever someone wants another configuration option down the road, I was thinking of changing Formatter.intialize to accept a hadoop Configuration object and let Formatters configure themselves. Good thinking. Convention-wise, we tend to make our own configuration objects instead of leveraging Hadoop Configuration objects. Examples include BatchWriterConfig , ClientConfiguration , ConditionalWriterConfig . I'd tend to go that route just for some consistency with the rest of the public API (assuming Formatters will eventually get pushed into the public API). WDYT?
          Hide
          Christopher Tubbs added a comment -

          I think we'd like to move away from Hadoop configuration objects whenever we're not interacting with Hadoop features/APIs, because we don't want to be tightly coupled to the Hadoop libraries, especially in our client code. We've been doing little things here and there to try to avoid further coupling.

          A couple of options which have been considered before:

          Properties - pro: built-in, generic, and flexible; con: not self-descriptive about possible options
          Commons Configuration - similar pros-cons as Properties, but maybe some additional tools/hierarchical parsing, options for file formats, etc.

          I've been playing around with another kind of abstraction which can add some self-descriptiveness to a generic configuration store like Properties/Commons Configuration, but it's far from fully fleshed out. The basic idea is that you'd specify a Class containing a set of typed Keys as the definition of your configuration options, and pass in your configuration source for it to parse, validate, and access. https://github.com/revelc/blazon

          Our current practice, however, tends to be writing fluent configuration builders which are narrowly scoped to the function in which they are being used (like BatchWriter, etc.)

          Show
          Christopher Tubbs added a comment - I think we'd like to move away from Hadoop configuration objects whenever we're not interacting with Hadoop features/APIs, because we don't want to be tightly coupled to the Hadoop libraries, especially in our client code. We've been doing little things here and there to try to avoid further coupling. A couple of options which have been considered before: Properties - pro: built-in, generic, and flexible; con: not self-descriptive about possible options Commons Configuration - similar pros-cons as Properties, but maybe some additional tools/hierarchical parsing, options for file formats, etc. I've been playing around with another kind of abstraction which can add some self-descriptiveness to a generic configuration store like Properties/Commons Configuration, but it's far from fully fleshed out. The basic idea is that you'd specify a Class containing a set of typed Keys as the definition of your configuration options, and pass in your configuration source for it to parse, validate, and access. https://github.com/revelc/blazon Our current practice, however, tends to be writing fluent configuration builders which are narrowly scoped to the function in which they are being used (like BatchWriter, etc.)
          Hide
          Matt Dailey added a comment -

          OK, I wanted to check in before my PR got too big. Against the 1.7 branch, the main changes I've made here:

          • New class FormatterConfig handles configuration of Formatter objects
          • To get around the Thread safety issues with DateFormat, I wrapped that in DateFormatGenerator which also enables a Formatter to have both Thread safety and per-instance DateFormat config.
          • FormatterConfig is taken by Formatter.initialize, which broke the interface for many classes throughout the non-public API, which were fixed

          Here's a preview of the changes I've made unsquashed since it's not ready for a PR yet (let me know if there's a better tool for this):
          https://github.com/apache/accumulo/compare/1.7...matthew-dailey:ACCUMULO-2493-1.7?expand=1

          Unit tests pass, but I'm getting a failure in the ReadWriteIT (will investigate).

          Any feedback is appreciated; feel free to let me know if an overhaul of the Formatters should really be put into a separate issue that this one depends on.

          Show
          Matt Dailey added a comment - OK, I wanted to check in before my PR got too big. Against the 1.7 branch, the main changes I've made here: New class FormatterConfig handles configuration of Formatter objects To get around the Thread safety issues with DateFormat, I wrapped that in DateFormatGenerator which also enables a Formatter to have both Thread safety and per-instance DateFormat config. FormatterConfig is taken by Formatter.initialize , which broke the interface for many classes throughout the non-public API, which were fixed Here's a preview of the changes I've made unsquashed since it's not ready for a PR yet (let me know if there's a better tool for this): https://github.com/apache/accumulo/compare/1.7...matthew-dailey:ACCUMULO-2493-1.7?expand=1 Unit tests pass, but I'm getting a failure in the ReadWriteIT (will investigate). Any feedback is appreciated; feel free to let me know if an overhaul of the Formatters should really be put into a separate issue that this one depends on.
          Hide
          Keith Turner added a comment -

          It may be nice to pass in a Connector and table name also. If passing these, then its more than config its also environmental info. So I tried changing the name of FormatterConfig to FormatterEnv below (but I could also see leaving the name as FormatterConfig). Also the DateFormatGenerator seems a lot like a Guava supplier (from the perspective of someone implementing a formatter). Also, maybe FormatterEnv should be an interface, with implementation package private.

            
            public interface FormatterEnv {
               String getTableName();
               Connector getConnector();
               Supplier<DateFormat> getDateFormatSupplier();
               public boolean willPrintTimestamps();
               public boolean willLimitShowLength();
               public int int getShowLengthToShow();
            }
          
            public interface Formatter extends Iterator<String> {
              void init(Iterable<Entry<Key,Value>> scanner, FormatterEnv env);
            }
          

          The reason I am thinking of passing a Connector is that it would allow an implementation to access custom table properties (added by ACCUMULO-2841). Connector is already in the public API and it would allow an implementation to do many other things like do lookups in another table for formatting.

          Also, I am not sure about the names willPrintTimestamps and willLimitShowLength. In my mind the implementation can optionally honor these (for example the implementation may never print timestamps or always print timestamps). Not sure what naming scheme would imply these are optional. For example FluoFormatter will always print the timestamp, regardless of the setting.

          Show
          Keith Turner added a comment - It may be nice to pass in a Connector and table name also. If passing these, then its more than config its also environmental info. So I tried changing the name of FormatterConfig to FormatterEnv below (but I could also see leaving the name as FormatterConfig). Also the DateFormatGenerator seems a lot like a Guava supplier (from the perspective of someone implementing a formatter). Also, maybe FormatterEnv should be an interface, with implementation package private. public interface FormatterEnv { String getTableName(); Connector getConnector(); Supplier<DateFormat> getDateFormatSupplier(); public boolean willPrintTimestamps(); public boolean willLimitShowLength(); public int int getShowLengthToShow(); } public interface Formatter extends Iterator< String > { void init(Iterable<Entry<Key,Value>> scanner, FormatterEnv env); } The reason I am thinking of passing a Connector is that it would allow an implementation to access custom table properties (added by ACCUMULO-2841 ). Connector is already in the public API and it would allow an implementation to do many other things like do lookups in another table for formatting. Also, I am not sure about the names willPrintTimestamps and willLimitShowLength . In my mind the implementation can optionally honor these (for example the implementation may never print timestamps or always print timestamps). Not sure what naming scheme would imply these are optional. For example FluoFormatter will always print the timestamp, regardless of the setting.
          Hide
          Mike Drob added a comment -

          Also the DateFormatGenerator seems a lot like a Guava supplier (from the perspective of someone implementing a formatter).

          I would prefer that we use java.util.function.Supplier over com.google.base.Supplier

          Show
          Mike Drob added a comment - Also the DateFormatGenerator seems a lot like a Guava supplier (from the perspective of someone implementing a formatter). I would prefer that we use java.util.function.Supplier over com.google.base.Supplier
          Hide
          Matt Dailey added a comment -

          I guess we can't use java.util.function.Supplier since that is introduced in Java 1.8, and the Accumulo source is only 1.7, right?

          Show
          Matt Dailey added a comment - I guess we can't use java.util.function.Supplier since that is introduced in Java 1.8, and the Accumulo source is only 1.7, right?
          Hide
          Keith Turner added a comment -

          Thats right. Following past precedents, bumping Accumulo 1.8 to Java 1.8 would require a vote on the dev list. W/o that need to stay at Java 1.7 for now.

          Show
          Keith Turner added a comment - Thats right. Following past precedents, bumping Accumulo 1.8 to Java 1.8 would require a vote on the dev list. W/o that need to stay at Java 1.7 for now.
          Hide
          Josh Elser added a comment -

          Triaging 1.7.1 tickets: What's your thought on getting a patch ready for this, Matt Dailey? Do you have an ETA?

          Show
          Josh Elser added a comment - Triaging 1.7.1 tickets: What's your thought on getting a patch ready for this, Matt Dailey ? Do you have an ETA?
          Hide
          Matt Dailey added a comment -

          I'll get back on this this weekend; so expect a patch on Monday 1/4/16.

          Show
          Matt Dailey added a comment - I'll get back on this this weekend; so expect a patch on Monday 1/4/16.
          Hide
          ASF GitHub Bot added a comment -

          GitHub user matthew-dailey opened a pull request:

          https://github.com/apache/accumulo/pull/61

          ACCUMULO-2493: Deprecated BinaryFormatter in favor of DefaultFormatter

          with FormatterConfig.

          • New class FormatterConfig handles configuration of Formatter objects
          • FormatterConfig is taken by Formatter.initialize, which broke the
            interface for many classes throughout the non-public API
          • Added DateFormatSupplier to let Formatters use DateFormat in a
            Thread-safe way
          • Removed code from ScanCommand and Shell tied to BinaryFormatter to use
            properly configured DefaultFormatter instead
          • Fixed bug where `scan -f [num] -fm [class]` would ignore Formatter
            class used in `-fm` and be overridden with BinaryFormatter

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/matthew-dailey/accumulo ACCUMULO-2493-1.7-squashed

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/accumulo/pull/61.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #61


          commit 8f60b4b1cb87f6bc315479d240e9e17b212a16a9
          Author: Matt Dailey <matthew.dailey1@gmail.com>
          Date: 2015-12-05T02:50:58Z

          ACCUMULO-2493: Deprecated BinaryFormatter in favor of DefaultFormatter
          with FormatterConfig.

          • New class FormatterConfig handles configuration of Formatter objects
          • FormatterConfig is taken by Formatter.initialize, which broke the
            interface for many classes throughout the non-public API
          • Added DateFormatSupplier to let Formatters use DateFormat in a
            Thread-safe way
          • Removed code from ScanCommand and Shell tied to BinaryFormatter to use
            properly configured DefaultFormatter instead
          • Fixed bug where `scan -f [num] -fm [class]` would ignore Formatter
            class used in `-fm` and be overridden with BinaryFormatter

          Show
          ASF GitHub Bot added a comment - GitHub user matthew-dailey opened a pull request: https://github.com/apache/accumulo/pull/61 ACCUMULO-2493 : Deprecated BinaryFormatter in favor of DefaultFormatter with FormatterConfig. New class FormatterConfig handles configuration of Formatter objects FormatterConfig is taken by Formatter.initialize, which broke the interface for many classes throughout the non-public API Added DateFormatSupplier to let Formatters use DateFormat in a Thread-safe way Removed code from ScanCommand and Shell tied to BinaryFormatter to use properly configured DefaultFormatter instead Fixed bug where `scan -f [num] -fm [class] ` would ignore Formatter class used in `-fm` and be overridden with BinaryFormatter You can merge this pull request into a Git repository by running: $ git pull https://github.com/matthew-dailey/accumulo ACCUMULO-2493 -1.7-squashed Alternatively you can review and apply these changes as the patch at: https://github.com/apache/accumulo/pull/61.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #61 commit 8f60b4b1cb87f6bc315479d240e9e17b212a16a9 Author: Matt Dailey <matthew.dailey1@gmail.com> Date: 2015-12-05T02:50:58Z ACCUMULO-2493 : Deprecated BinaryFormatter in favor of DefaultFormatter with FormatterConfig. New class FormatterConfig handles configuration of Formatter objects FormatterConfig is taken by Formatter.initialize, which broke the interface for many classes throughout the non-public API Added DateFormatSupplier to let Formatters use DateFormat in a Thread-safe way Removed code from ScanCommand and Shell tied to BinaryFormatter to use properly configured DefaultFormatter instead Fixed bug where `scan -f [num] -fm [class] ` would ignore Formatter class used in `-fm` and be overridden with BinaryFormatter
          Hide
          Matt Dailey added a comment -

          I liked the Supplier interface idea, so I refactored the class to be called DateFormatSupplier extends ... implements Supplier<DateFormat>.

          The FormatterEnv change I was on the fence about, and ended up not including it in my pull request. I like the idea of extra power for the Formatter via context, but adding a dependency to a Connector ties Formatters to only be used in that context. Formatters wouldn't be usable to just format entries, like in a MapReduce context where there is no active Connector.

          Show
          Matt Dailey added a comment - I liked the Supplier interface idea, so I refactored the class to be called DateFormatSupplier extends ... implements Supplier<DateFormat> . The FormatterEnv change I was on the fence about, and ended up not including it in my pull request. I like the idea of extra power for the Formatter via context, but adding a dependency to a Connector ties Formatters to only be used in that context. Formatters wouldn't be usable to just format entries, like in a MapReduce context where there is no active Connector.
          Hide
          ASF GitHub Bot added a comment -

          Github user joshelser commented on the pull request:

          https://github.com/apache/accumulo/pull/61#issuecomment-168897649

          This looks really good! Lots of new tests, new docs, and BinaryFormatter is cleaned up nicely by the FormatterConfig.

          Anyone else have any input before I run some tests and merge it in?

          Show
          ASF GitHub Bot added a comment - Github user joshelser commented on the pull request: https://github.com/apache/accumulo/pull/61#issuecomment-168897649 This looks really good! Lots of new tests, new docs, and BinaryFormatter is cleaned up nicely by the FormatterConfig. Anyone else have any input before I run some tests and merge it in?
          Hide
          Josh Elser added a comment -

          Ran this through Yetus for funsies:

          Full console: https://paste.apache.org/d29E

          +1 overall
          
           ____                              _
          / ___| _   _  ___ ___ ___  ___ ___| |
          \___ \| | | |/ __/ __/ _ \/ __/ __| |
           ___) | |_| | (_| (_|  __/\__ \__ \_|
          |____/ \__,_|\___\___\___||___/___(_)
          
          
          Vote Subsystem Runtime Comment
          ============================================================================
          +1 @author 0m 00s The patch does not contain any @author
                tags.
          +1 test4tests 0m 00s The patch appears to include 11 new or
                modified test files.
          +1 mvninstall 1m 58s 1.7 passed
          +1 compile 1m 24s 1.7 passed
          +1 checkstyle 1m 01s 1.7 passed
          +1 mvneclipse 1m 06s 1.7 passed
          +1 findbugs 3m 55s 1.7 passed
          +1 javadoc 1m 17s 1.7 passed
          +1 mvninstall 1m 28s the patch passed
          +1 compile 1m 22s the patch passed
          +1 javac 1m 22s the patch passed
          +1 checkstyle 1m 03s the patch passed
          +1 mvneclipse 1m 06s the patch passed
          +1 whitespace 0m 00s Patch has no whitespace issues.
          +1 findbugs 4m 09s the patch passed
          +1 javadoc 1m 27s the patch passed
          +1 asflicense 0m 36s Patch does not generate ASF License
                warnings.
              22m 39s
          Subsystem Report/Notes

          ============================================================================

          GITHUB PR https://github.com/apache/accumulo/pull/61
          JIRA Issue ACCUMULO-2493
          Optional Tests asflicense javac javadoc unit findbugs checkstyle compile
          uname Darwin hw10447.local 14.5.0 Darwin Kernel Version 14.5.0: Tue Sep 1 21:23:09 PDT 2015; root:xnu-2782.50.1~1/RELEASE_X86_64 x86_64
          Build tool maven
          Personality /usr/local/lib/yetus-0.1.0/lib/precommit/personality/Accumulo.sh
          git revision 1.7 / ff08336
          findbugs v3.0.1
          modules C: core server/base server/tracer shell U: .
          Powered by Apache Yetus 0.1.0 http://yetus.apache.org

          I do think I forgot to add the "run-tests" flag, however...

          Show
          Josh Elser added a comment - Ran this through Yetus for funsies: Full console: https://paste.apache.org/d29E +1 overall ____ _ / ___| _ _ ___ ___ ___ ___ ___| | \___ \| | | |/ __/ __/ _ \/ __/ __| | ___) | |_| | (_| (_| __/\__ \__ \_| |____/ \__,_|\___\___\___||___/___(_) Vote Subsystem Runtime Comment ============================================================================ +1 @author 0m 00s The patch does not contain any @author       tags. +1 test4tests 0m 00s The patch appears to include 11 new or       modified test files. +1 mvninstall 1m 58s 1.7 passed +1 compile 1m 24s 1.7 passed +1 checkstyle 1m 01s 1.7 passed +1 mvneclipse 1m 06s 1.7 passed +1 findbugs 3m 55s 1.7 passed +1 javadoc 1m 17s 1.7 passed +1 mvninstall 1m 28s the patch passed +1 compile 1m 22s the patch passed +1 javac 1m 22s the patch passed +1 checkstyle 1m 03s the patch passed +1 mvneclipse 1m 06s the patch passed +1 whitespace 0m 00s Patch has no whitespace issues. +1 findbugs 4m 09s the patch passed +1 javadoc 1m 27s the patch passed +1 asflicense 0m 36s Patch does not generate ASF License       warnings.     22m 39s Subsystem Report/Notes ============================================================================ GITHUB PR https://github.com/apache/accumulo/pull/61 JIRA Issue ACCUMULO-2493 Optional Tests asflicense javac javadoc unit findbugs checkstyle compile uname Darwin hw10447.local 14.5.0 Darwin Kernel Version 14.5.0: Tue Sep 1 21:23:09 PDT 2015; root:xnu-2782.50.1~1/RELEASE_X86_64 x86_64 Build tool maven Personality /usr/local/lib/yetus-0.1.0/lib/precommit/personality/Accumulo.sh git revision 1.7 / ff08336 findbugs v3.0.1 modules C: core server/base server/tracer shell U: . Powered by Apache Yetus 0.1.0 http://yetus.apache.org I do think I forgot to add the "run-tests" flag, however...
          Hide
          Josh Elser added a comment -

          I'll try to rebase+merge this in. Keith Turner, did you want to review this before I do?

          Show
          Josh Elser added a comment - I'll try to rebase+merge this in. Keith Turner , did you want to review this before I do?
          Hide
          Keith Turner added a comment -

          I took a look at it looks good. I noticed something minor. Wondering if DateStringFormatter should be deprecated like BinaryFormatter?

          Show
          Keith Turner added a comment - I took a look at it looks good. I noticed something minor. Wondering if DateStringFormatter should be deprecated like BinaryFormatter?
          Hide
          Josh Elser added a comment -

          Wondering if DateStringFormatter should be deprecated like BinaryFormatter?

          Makes sense given the nice javadoc explanation. Not in public API, so we're good to add the Deprecated now. I can add that on rebase/merge

          Show
          Josh Elser added a comment - Wondering if DateStringFormatter should be deprecated like BinaryFormatter? Makes sense given the nice javadoc explanation. Not in public API, so we're good to add the Deprecated now. I can add that on rebase/merge
          Hide
          Matt Dailey added a comment -

          I think the only reason I didn't deprecate DateStringFormatter was because it can still be used when specifying a formatter with the -fm flag when scanning/grepping in the shell. However, deprecation should not affect the output seen by users on the shell, so it makes sense to deprecate the class so others do not rely on it in code.

          Show
          Matt Dailey added a comment - I think the only reason I didn't deprecate DateStringFormatter was because it can still be used when specifying a formatter with the -fm flag when scanning/grepping in the shell. However, deprecation should not affect the output seen by users on the shell, so it makes sense to deprecate the class so others do not rely on it in code.
          Hide
          Christopher Tubbs added a comment -

          In the spirit of semver, it might be better to drop the 1.7 fixVersion for this, if it doesn't clearly address a bug that affects 1.7 users or help with future 1.7 bug fixes. If it's just trying to make the internals better, to ease future development, then it's probably better targeted to 1.8/master.

          However, I haven't looked closely enough at this issue to know which is the case. It does seem that there's a risk it will change the behavior of the shell a bit for 1.7 users, but perhaps the bugs it addresses make that risk acceptable? I'll defer to others. It was just a point I thought might be worth raising.

          Show
          Christopher Tubbs added a comment - In the spirit of semver, it might be better to drop the 1.7 fixVersion for this, if it doesn't clearly address a bug that affects 1.7 users or help with future 1.7 bug fixes. If it's just trying to make the internals better, to ease future development, then it's probably better targeted to 1.8/master. However, I haven't looked closely enough at this issue to know which is the case. It does seem that there's a risk it will change the behavior of the shell a bit for 1.7 users, but perhaps the bugs it addresses make that risk acceptable? I'll defer to others. It was just a point I thought might be worth raising.
          Hide
          Josh Elser added a comment -

          Ok, I'm dropping 1.7 from the fixVersion and pushing this.

          Show
          Josh Elser added a comment - Ok, I'm dropping 1.7 from the fixVersion and pushing this.
          Hide
          Josh Elser added a comment -

          Sorry for the last minute branch-switch, Matt. Thanks for the contribution nonetheless!

          Show
          Josh Elser added a comment - Sorry for the last minute branch-switch, Matt. Thanks for the contribution nonetheless!
          Hide
          ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/accumulo/pull/61

          Show
          ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/accumulo/pull/61

            People

            • Assignee:
              Matt Dailey
              Reporter:
              Mike Drob
            • Votes:
              0 Vote for this issue
              Watchers:
              7 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 - 40m
                40m

                  Development