[
https://issues.apache.org/jira/browse/HBASE-15702?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15266119#comment-15266119
]
Hiroshi Ikeda commented on HBASE-15702:
---------------------------------------
{code}
- private static volatile NonceGenerator nonceGenerator = null;
...skip...
+ private volatile NonceGenerator nonceGenerator = null;
{code}
I didn't realize but you are right that the "volatile" is still required for a
test method. That's annoying but I have no idea. Also removing "static" causes
semantics change but anyone including tests seems not to expect that field has
global effect.
{code}
- static class NoNonceGenerator implements NonceGenerator {
+ final static class NoNonceGenerator implements NonceGenerator {
+ static NoNonceGenerator INSTANCE = new NoNonceGenerator();
{code}
The "final" should be appended to the field rather than the class.
{code}
+ public void setNonceGenerator(NonceGenerator nonceGenerator) {
+ this.nonceGenerator = nonceGenerator;
+ }
{code}
The "public" is not required. Even it is safe to append "@VisibleForTesting"
for future developers.
{code}
@InterfaceAudience.Private
public class PerClientRandomNonceGenerator implements NonceGenerator {
...skip...
+ static final PerClientRandomNonceGenerator INSTANCE =
+ new PerClientRandomNonceGenerator();
{code}
The visibility is inconsistent. Make the field INSTANCE "public", or make the
class package-private (removing "public").
HBase often names a static variable with capital letters, even if it is never
regarded as a constant :(
> Improve PerClientRandomNonceGenerator
> -------------------------------------
>
> Key: HBASE-15702
> URL: https://issues.apache.org/jira/browse/HBASE-15702
> Project: HBase
> Issue Type: Improvement
> Reporter: Hiroshi Ikeda
> Priority: Trivial
> Fix For: 2.0.0
>
> Attachments: HBASE-15702.patch, HBASE-15702_v1.patch,
> HBASE-15702_v2.patch, HBASE-15702_v3.patch, HBASE-15702_v4.patch
>
>
> PerClientRandomNonceGenerator can be exposed to all the threads via the
> static field ConnectionManager.nonceGenerator, but
> PerClientRandomNonceGenerator uses Random, which should be ThreadLocalRandom
> or something. (See javadoc of Random.)
> Moreover, ConnectionManager creates or refers the singleton instance of
> PerClientThreadLocalRandom with a lock or volatile, but it should be created
> as a static final field in PerClientThreadLocalRandom itself, and the
> creation will be postponed until the field is actually refereed and the class
> is being initialized.
> The same can be said for ConnectionManager.NoNonceGenerator.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
