accumulo-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (ACCUMULO-1972) Range constructors call overridable method
Date Sat, 09 Dec 2017 00:05:00 GMT

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

Mime
View raw message