Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-23969 Meta browser should show all info columns #1485

Merged
merged 7 commits into from
May 13, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused import.

import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
Expand Down Expand Up @@ -384,32 +385,42 @@ public static List<RegionInfo> getMergeRegions(Connection connection, byte[] reg
}

/**
* @return Deserialized regioninfo values taken from column values that match
* @return Deserialized values of <qualifier,regioninfo> pairs taken from column values that match
* the regex 'info:merge.*' in array of <code>cells</code>.
*/
@Nullable
public static List<RegionInfo> getMergeRegions(Cell [] cells) {
public static Map<String, RegionInfo> getMergeRegionsWithName(Cell [] cells) {
if (cells == null) {
return null;
}
List<RegionInfo> regionsToMerge = null;
Map<String, RegionInfo> regionsToMerge = null;
for (Cell cell: cells) {
if (!isMergeQualifierPrefix(cell)) {
continue;
}
// Ok. This cell is that of a info:merge* column.
RegionInfo ri = RegionInfo.parseFromOrNull(cell.getValueArray(), cell.getValueOffset(),
cell.getValueLength());
cell.getValueLength());
if (ri != null) {
if (regionsToMerge == null) {
regionsToMerge = new ArrayList<>();
regionsToMerge = new LinkedHashMap<>();
}
regionsToMerge.add(ri);
regionsToMerge.put(Bytes.toString(CellUtil.cloneQualifier(cell)), ri);
}
}
return regionsToMerge;
}

/**
* @return Deserialized regioninfo values taken from column values that match
* the regex 'info:merge.*' in array of <code>cells</code>.
*/
@Nullable
public static List<RegionInfo> getMergeRegions(Cell [] cells) {
Map<String, RegionInfo> mergeRegionsWithName = getMergeRegionsWithName(cells);
return (mergeRegionsWithName == null) ? null : new ArrayList<>(mergeRegionsWithName.values());
}

/**
* @return True if any merge regions present in <code>cells</code>; i.e.
* the column in <code>cell</code> matches the regex 'info:merge.*'.
Expand Down Expand Up @@ -873,8 +884,7 @@ static byte[] getRegionStateColumn(int replicaId) {
* @param replicaId the replicaId of the region
* @return a byte[] for sn column qualifier
*/
@VisibleForTesting
static byte[] getServerNameColumn(int replicaId) {
public static byte[] getServerNameColumn(int replicaId) {
return replicaId == 0 ? HConstants.SERVERNAME_QUALIFIER
: Bytes.toBytes(HConstants.SERVERNAME_QUALIFIER_STR + META_REPLICA_ID_DELIMITER
+ String.format(RegionInfo.REPLICA_ID_FORMAT, replicaId));
Expand Down Expand Up @@ -966,6 +976,33 @@ public static ServerName getServerName(final Result r, final int replicaId) {
}
}

/**
* Returns the {@link ServerName} from catalog table {@link Result} where the region is
* transitioning on. It should be the same as {@link MetaTableAccessor#getServerName(Result,int)}
* if the server is at OPEN state.
*
* @param r Result to pull the transitioning server name from
* @return A ServerName instance or {@link MetaTableAccessor#getServerName(Result,int)}
* if necessary fields not found or empty.
*/
@Nullable
public static ServerName getTargetServerName(final Result r, final int replicaId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is rationale for moving this method? How is it MetaTableAccessor material?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. and not private to RegoinStateStore where it was originally?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saintstack This method is used so far by both RegionStateStore and RegionReplicaInfo (new for table.jsp). So it can not be private to RegoinStateStore. We can keep it there and make it public, or we move to MetaTableAccessor which is more generic place for Read/write hbase:meta operations. I see all getXXColumn and getXX stuff are static methods in MetaTableAccessor which are similar to what this method does. So I moved the logic reading "info:sn" columns to MetaTableAccessor.

This also came from my coding experience. When I write the first patch, I implemented my version in MetaTableAccessor instead of calling the implementation in RegoinStateStore. I simply did not know existing logic is already someplace there. Meanwhile, both RegionStateStore and MetaTableAccessor have their own getServerNameColumn() method. I guess someone previously may have missed existing implementation as I did :) So overall I prefer the central place of getting columns and data from hbase:meta. But I understand this move makes it public in client module. If that is a concern, I can move it back.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long-stale conversation, perhaps...

I agree with @liuml07 on this one -- it's a good thing to have a centralized place for encapsulating the serialization details of storing and retrieving content from meta. MetaTableAccessor seems like the place for it.

final Cell cell = r.getColumnLatestCell(HConstants.CATALOG_FAMILY,
getServerNameColumn(replicaId));
if (cell == null || cell.getValueLength() == 0) {
RegionLocations locations = MetaTableAccessor.getRegionLocations(r);
if (locations != null) {
HRegionLocation location = locations.getRegionLocation(replicaId);
if (location != null) {
return location.getServerName();
}
}
return null;
}
return ServerName.parseServerName(Bytes.toString(cell.getValueArray(), cell.getValueOffset(),
cell.getValueLength()));
}

/**
* The latest seqnum that the server writing to meta observed when opening the region.
* E.g. the seqNum when the result of {@link #getServerName(Result, int)} was written.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -500,11 +500,15 @@ public enum OperationStatusCode {

public static final byte [] SERVERNAME_QUALIFIER = Bytes.toBytes(SERVERNAME_QUALIFIER_STR);

/** The lower-half split region column qualifier string. */
public static final String SPLITA_QUALIFIER_STR = "splitA";
/** The lower-half split region column qualifier */
public static final byte [] SPLITA_QUALIFIER = Bytes.toBytes("splitA");
public static final byte [] SPLITA_QUALIFIER = Bytes.toBytes(SPLITA_QUALIFIER_STR);

/** The upper-half split region column qualifier String. */
public static final String SPLITB_QUALIFIER_STR = "splitB";
/** The upper-half split region column qualifier */
public static final byte [] SPLITB_QUALIFIER = Bytes.toBytes("splitB");
public static final byte [] SPLITB_QUALIFIER = Bytes.toBytes(SPLITB_QUALIFIER_STR);

/**
* Merge qualifier prefix.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ private void visitMetaEntry(final RegionStateVisitor visitor, final Result resul
final State state = getRegionState(result, regionInfo);

final ServerName lastHost = hrl.getServerName();
final ServerName regionLocation = getRegionServer(result, replicaId);
ServerName regionLocation = MetaTableAccessor.getTargetServerName(result, replicaId);
final long openSeqNum = hrl.getSeqNum();

// TODO: move under trace, now is visible for debugging
Expand Down Expand Up @@ -199,7 +199,7 @@ private void updateUserRegionLocation(RegionInfo regionInfo, State state,
put.add(CellBuilderFactory.create(CellBuilderType.SHALLOW_COPY)
.setRow(put.getRow())
.setFamily(HConstants.CATALOG_FAMILY)
.setQualifier(getServerNameColumn(replicaId))
.setQualifier(MetaTableAccessor.getServerNameColumn(replicaId))
.setTimestamp(put.getTimestamp())
.setType(Cell.Type.Put)
.setValue(Bytes.toBytes(regionLocation.getServerName()))
Expand Down Expand Up @@ -299,42 +299,6 @@ private TableDescriptor getDescriptor(TableName tableName) throws IOException {
return master.getTableDescriptors().get(tableName);
}

// ==========================================================================
// Server Name
// ==========================================================================

/**
* Returns the {@link ServerName} from catalog table {@link Result}
* where the region is transitioning. It should be the same as
* {@link MetaTableAccessor#getServerName(Result,int)} if the server is at OPEN state.
* @param r Result to pull the transitioning server name from
* @return A ServerName instance or {@link MetaTableAccessor#getServerName(Result,int)}
* if necessary fields not found or empty.
*/
static ServerName getRegionServer(final Result r, int replicaId) {
final Cell cell = r.getColumnLatestCell(HConstants.CATALOG_FAMILY,
getServerNameColumn(replicaId));
if (cell == null || cell.getValueLength() == 0) {
RegionLocations locations = MetaTableAccessor.getRegionLocations(r);
if (locations != null) {
HRegionLocation location = locations.getRegionLocation(replicaId);
if (location != null) {
return location.getServerName();
}
}
return null;
}
return ServerName.parseServerName(Bytes.toString(cell.getValueArray(),
cell.getValueOffset(), cell.getValueLength()));
}

private static byte[] getServerNameColumn(int replicaId) {
return replicaId == 0
? HConstants.SERVERNAME_QUALIFIER
: Bytes.toBytes(HConstants.SERVERNAME_QUALIFIER_STR + META_REPLICA_ID_DELIMITER
+ String.format(RegionInfo.REPLICA_ID_FORMAT, replicaId));
}

// ==========================================================================
// Region State
// ==========================================================================
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,22 @@
*/
package org.apache.hadoop.hbase.master.webapp;

import static org.apache.hbase.thirdparty.org.apache.commons.collections4.ListUtils.emptyIfNull;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few unused imports to cleanup.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have fixed this.

p.s. For a PR to Hadoop, I usually check the "checkstyle" report by Yetus and update the patch accordingly. I see the Yetus for HBase QA does not report "checkstyle" warnings in the comment table. I found there are report links in the "Console output" though. I'll follow there next time. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to complications around multi-jdk support, we have Yetus run in 3 different invocations. Looks like the checkstyle check gave you a +1.


import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.commons.lang3.builder.HashCodeBuilder;
import org.apache.commons.lang3.builder.ToStringBuilder;
import org.apache.commons.lang3.builder.ToStringStyle;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.HRegionLocation;
import org.apache.hadoop.hbase.MetaTableAccessor;
import org.apache.hadoop.hbase.RegionLocations;
Expand All @@ -34,6 +42,7 @@
import org.apache.hadoop.hbase.master.RegionState;
import org.apache.hadoop.hbase.master.assignment.RegionStateStore;
import org.apache.hadoop.hbase.util.Bytes;
import org.apache.hadoop.hbase.util.PairOfSameType;
import org.apache.yetus.audience.InterfaceAudience;

/**
Expand All @@ -45,6 +54,11 @@ public final class RegionReplicaInfo {
private final RegionInfo regionInfo;
private final RegionState.State regionState;
private final ServerName serverName;
private final long seqNum;
/** See {@link org.apache.hadoop.hbase.HConstants#SERVERNAME_QUALIFIER_STR}. */
private final ServerName targetServerName;
private final Map<String, RegionInfo> mergeRegionInfo;
private final Map<String, RegionInfo> splitRegionInfo;

private RegionReplicaInfo(final Result result, final HRegionLocation location) {
this.row = result != null ? result.getRow() : null;
Expand All @@ -53,6 +67,26 @@ private RegionReplicaInfo(final Result result, final HRegionLocation location) {
? RegionStateStore.getRegionState(result, regionInfo)
: null;
this.serverName = location != null ? location.getServerName() : null;
this.seqNum = (location != null) ? location.getSeqNum() : HConstants.NO_SEQNUM;
this.targetServerName = (result != null && regionInfo != null)
? MetaTableAccessor.getTargetServerName(result, regionInfo.getReplicaId())
: null;
this.mergeRegionInfo = (result != null)
? MetaTableAccessor.getMergeRegionsWithName(result.rawCells())
: null;

if (result != null) {
PairOfSameType<RegionInfo> daughterRegions = MetaTableAccessor.getDaughterRegions(result);
this.splitRegionInfo = new LinkedHashMap<>();
if (daughterRegions.getFirst() != null) {
splitRegionInfo.put(HConstants.SPLITA_QUALIFIER_STR, daughterRegions.getFirst());
}
if (daughterRegions.getSecond() != null) {
splitRegionInfo.put(HConstants.SPLITB_QUALIFIER_STR, daughterRegions.getSecond());
}
} else {
this.splitRegionInfo = null;
}
}

public static List<RegionReplicaInfo> from(final Result result) {
Expand Down Expand Up @@ -102,6 +136,22 @@ public ServerName getServerName() {
return serverName;
}

public long getSeqNum() {
return seqNum;
}

public ServerName getTargetServerName() {
return targetServerName;
}

public Map<String, RegionInfo> getMergeRegionInfo() {
return mergeRegionInfo;
}

public Map<String, RegionInfo> getSplitRegionInfo() {
return splitRegionInfo;
}

@Override
public boolean equals(Object other) {
if (this == other) {
Expand All @@ -119,6 +169,10 @@ public boolean equals(Object other) {
.append(regionInfo, that.regionInfo)
.append(regionState, that.regionState)
.append(serverName, that.serverName)
.append(seqNum, that.seqNum)
.append(targetServerName, that.targetServerName)
.append(mergeRegionInfo, that.mergeRegionInfo)
.append(splitRegionInfo, that.splitRegionInfo)
.isEquals();
}

Expand All @@ -129,15 +183,24 @@ public int hashCode() {
.append(regionInfo)
.append(regionState)
.append(serverName)
.append(seqNum)
.append(targetServerName)
.append(mergeRegionInfo)
.append(splitRegionInfo)
.toHashCode();
}

@Override public String toString() {
@Override
public String toString() {
return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE)
.append("row", Bytes.toStringBinary(row))
.append("regionInfo", regionInfo)
.append("regionState", regionState)
.append("serverName", serverName)
.append("seqNum", seqNum)
.append("transitioningOnServerName", targetServerName)
.append("merge*", mergeRegionInfo)
.append("split*", splitRegionInfo)
.toString();
}
}