[
https://issues.apache.org/jira/browse/HBASE-4153?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13105868#comment-13105868
]
stack commented on HBASE-4153:
------------------------------
Nits:
{code}
+ * RegionsAlreadyInTransitionException. The above message is used
+ * to extract the status in the master.
{code}
The comment 'above message' doesn't seem to refer to anything?
Do you want to say TRANSITIONING in below?
{code}
+ private static final String ALREADY_TRANSITING = "for the region we are
already trying to ";
{code}
This is odd Ram. Why not just write it out? Its odd having define for SPACE
in particular:
{code}
+ boolean actionType = this.regionsInTransitionInRS.get(encodedName);
+ throw new RegionAlreadyInTransitionException(REGIONSERVER + SPACE +
+ this.getServerName() + RECEIVED + SPACE + OPEN + SPACE +
+ ALREADY_TRANSITING + (actionType ? OPEN : CLOSE) + "; " +
+ region.getRegionNameAsString());
{code}
Otherwise, these more detailed exceptions are great.
Except, are we repeating their creation? Why not do the creation of the
exception in a method and then you might not need the defines since the
exception construction is done once only?
Is it important to change this:
{code}
- public Set<byte[]> getRegionsInTransitionInRS() {
+ public ConcurrentSkipListMap<byte[], Boolean> getRegionsInTransitionInRS() {
return this.regionsInTransitionInRS;
}
{code}
You could just return the keys? No problem if its important to return whole
map (Should this map be going outside of this class? Will there be damage done
if someone external to this class alters its content?)
So here:
{code}
- this.regionsInTransitionInRS.add(region.getEncodedNameAsBytes());
+ this.regionsInTransitionInRS.putIfAbsent(encodedName, true);
{code}
What if there is something there already? Previous we'd overwrite whatever it
was. Now we'll not overwrite. Is that a problem?
Why redefine String over here in AssignmentManager:
{code}
+ //String to compare the RegionsAlreadyInTransition from RS
+ private static final String ALREADY_TRANSITING = "for the region we are " +
+ "already trying to ";
{code}
Why not just use the define made already? (Same for later in this class where
you use it again and where you look for 'OPEN'... use the define?
FYI, you could write the below as: return test? A: B;
{code}
+ if (t.getMessage().contains(ALREADY_TRANSITING+"OPEN")) {
+ return RegionState.State.PENDING_OPEN;
+ }
+ return RegionState.State.PENDING_CLOSE;
{code}
Yeah, do you need to let out the new Map? You could just leave it as a Set as
it was before especially if no one else cares about the values. Just return
Map.getKeys().
Otherwise patch is good.
> Handle RegionAlreadyInTransitionException in AssignmentManager
> --------------------------------------------------------------
>
> Key: HBASE-4153
> URL: https://issues.apache.org/jira/browse/HBASE-4153
> Project: HBase
> Issue Type: Improvement
> Affects Versions: 0.92.0
> Reporter: Jean-Daniel Cryans
> Assignee: ramkrishna.s.vasudevan
> Fix For: 0.92.0
>
> Attachments: 4153-v3.txt, HBASE-4153_1.patch, HBASE-4153_2.patch
>
>
> Comment from Stack over in HBASE-3741:
> {quote}
> Question: Looking at this patch again, if we throw a
> RegionAlreadyInTransitionException, won't we just assign the region elsewhere
> though RegionAlreadyInTransitionException in at least one case here is saying
> that the region is already open on this regionserver?
> {quote}
> Indeed looking at the code it's going to be handled the same way other
> exceptions are. Need to add special cases for assign and unassign.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
