[jira] [Commented] (ACCUMULO-1972) Range constructors call overridable method

["Christopher Tubbs (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/ACCUMULO-1972?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16284450#comment-16284450 ] 

Christopher Tubbs commented on ACCUMULO-1972:
---------------------------------------------

[~coffeethulhu], thanks, I have a few comments before applying your updated patch.

* The constructor should be updated to reference the new {{beforeStartKeyImpl}} instead of {{beforeStartKey}}. That would fix the original problem, because the private method can't be overridden.
* I don't think we need the new private method for {{afterEndKeyImpl}}, because that was never called in the constructor.
* I think the original javadocs on the original public methods do not need to be changed. We want the javadocs on the public methods to reflect the user experience, not the internal implementation. The user does not need to know that it calls the private method.
* If you strip trailing whitespace off of the lines in your patch, and run {{mvn clean package -DskipTests}}, your code will be formatted using our built-in code formatter during the build. I can also do this before applying the patch, but I wanted to let you know about it.

Also, a hint for creating your patch: if you create a "git-formatted" patch (for example: {{git format-patch HEAD~1}}) to create a patch file, instead of {{git diff}}, the patch will include your authorship information, so you will get credit when we apply the patch. If it's more convenient, you can also submit a pull request against the 1.7 branch at https://github.com/apache/accumulo ; I can still give you credit if I create the git commit, but only by mentioning your name in the log message. Whatever you prefer is fine with me, but thought I'd mention it, so you had the opportunity to get credit in the git commit history of Accumulo. :)


> Range constructors call overridable method
> ------------------------------------------
>
>                 Key: ACCUMULO-1972
>                 URL: https://issues.apache.org/jira/browse/ACCUMULO-1972
>             Project: Accumulo
>          Issue Type: Bug
>    Affects Versions: 1.4.4, 1.5.0
>            Reporter: Bill Havanki
>            Assignee: Matthew Dinep
>            Priority: Minor
>              Labels: newbie
>             Fix For: 1.7.4, 1.8.2, 2.0.0
>
>         Attachments: accumulo-1972.patch, accumulo-1972_2.patch
>
>
> Several {{Range}} constructors call {{Range.beforeStartKey()}}, which is not final. This is dangerous:
> bq. The superclass constructor runs before the subclass constructor, so the overriding method in the subclass will get invoked before the subclass constructor has run. If the overriding method depends on any initialization performed by the subclass constructor, the method will not behave as expected.  ??Item 17, Effective Java Vol. 2, Bloch??
> If {{beforeStartKey()}} cannot be made final, the code should be refactored to make the constructors safe.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)