[
https://issues.apache.org/jira/browse/HBASE-22031?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16803006#comment-16803006
]
Xiang Li edited comment on HBASE-22031 at 3/27/19 4:22 PM:
-----------------------------------------------------------
Hi [~xucang] yes, it is a good question (for me) that if it worths here, or
necessary.
The shallow copy trades safety for a better performance: It is safe if we deep
copy (allocate new TreeSets and then make a copy) for tables and servers
inputted. The update on the inputs will not affect RSGroupInfo instance. But as
a trace-off, the allocation of TreeSet is heavy.
By using shallow copy, RSGroupInfo instance points the the input ones, rather
than making a copy within itself. It is fast but the subsequent update on the
inputs will also affect that RSGroupInfo instance.
If the code followed does not change the input servers and tables, it is safe
to do the shallow copy. I will review the code in the context to make sure it
is safe to do that.
was (Author: water):
Hi [~xucang] yes, it is a good question (for me) that if it worths here.
The shallow copy trades safety for a better performance: It is safe if we deep
copy (allocate new TreeSets and then make a copy) for tables and servers
inputted. The update on the inputs will not affect RSGroupInfo instance. But as
a trace-off, the allocation of TreeSet is heavy.
By using shallow copy, RSGroupInfo instance points the the input ones, rather
than making a copy within itself. It is fast but the subsequent update on the
inputs will also affect that RSGroupInfo instance.
If the code followed does not change the input servers and tables, it is safe
to do the shallow copy. I will review the code in the context to make sure it
is safe to do that.
> Provide RSGroupInfo with a new constructor using shallow copy
> -------------------------------------------------------------
>
> Key: HBASE-22031
> URL: https://issues.apache.org/jira/browse/HBASE-22031
> Project: HBase
> Issue Type: Improvement
> Components: rsgroup
> Reporter: Xiang Li
> Assignee: Xiang Li
> Priority: Minor
> Attachments: HBASE-22031.master.000.patch,
> HBASE-22031.master.001.patch, HBASE-22031.master.002.patch
>
>
> As for org.apache.hadoop.hbase.rsgroup.RSGroupInfo, the following constructor
> performs deep copies on both servers and tables inputed.
> {code:title=hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java.java|borderStyle=solid}
> RSGroupInfo(String name, SortedSet<Address> servers, SortedSet<TableName>
> tables) {
> this.name = name;
> this.servers = (servers == null) ? new TreeSet<>() : new TreeSet<>(servers);
> this.tables = (tables == null) ? new TreeSet<>() : new TreeSet<>(tables);
> }
> {code}
> The constructor of TreeSet is heavy and I think it is better to have a new
> constructor with shallow copy and it could be used at least in
> {code:title=hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java|borderStyle=solid}
> private synchronized void refresh(boolean forceOnline) throws IOException {
> ...
> groupList.add(new RSGroupInfo(RSGroupInfo.DEFAULT_GROUP,
> getDefaultServers(), orphanTables));
> ...
> {code}
> It is not needed to allocate new TreeSet to deep copy the output of
> getDefaultServers() and orphanTables, both of which are allocated in the near
> context and not updated in the code followed. So it is safe to make a shadow
> copy here.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
