[
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)
