keith-turner commented on code in PR #3482:
URL: https://github.com/apache/accumulo/pull/3482#discussion_r1227004983
##########
core/src/main/java/org/apache/accumulo/core/metadata/TServerInstance.java:
##########
@@ -34,56 +36,74 @@
*/
public class TServerInstance implements Comparable<TServerInstance> {
+ private static final String FORMAT = "%s#%s#%s";
+
private final HostAndPort hostAndPort;
private final String hostPort;
private final String session;
- private final String hostPortSession;
+ private final String hostPortSessionGroup;
+ private final String group;
+
+ public static TServerInstance fromString(String val) {
+ Objects.requireNonNull(val, "String value for TServerInstance cannot be
null");
+ String[] parts = val.split("#");
+ if (parts.length != 3) {
+ // could throw IllegalArgumentException, but IllegalStateException is
something
+ // that is already expected in a large portion of the codebase.
+ throw new IllegalStateException(
+ "Supplied TServerInstance string: " + val + " does not follow
format: " + FORMAT);
+ }
+ return new TServerInstance(parts[0], parts[1], parts[2]);
+ }
- public TServerInstance(HostAndPort address, String session) {
+ public TServerInstance(HostAndPort address, String session, String group) {
this.hostAndPort = address;
this.session = session;
this.hostPort = hostAndPort.toString();
- this.hostPortSession = hostPort + "[" + session + "]";
+ this.group = group;
Review Comment:
Is it expected that group can be null? If so is null expected to make it
through serialization and deserialization of null?
##########
core/src/main/java/org/apache/accumulo/core/metadata/TServerInstance.java:
##########
@@ -34,56 +36,74 @@
*/
public class TServerInstance implements Comparable<TServerInstance> {
+ private static final String FORMAT = "%s#%s#%s";
+
private final HostAndPort hostAndPort;
private final String hostPort;
private final String session;
- private final String hostPortSession;
+ private final String hostPortSessionGroup;
+ private final String group;
+
+ public static TServerInstance fromString(String val) {
+ Objects.requireNonNull(val, "String value for TServerInstance cannot be
null");
+ String[] parts = val.split("#");
+ if (parts.length != 3) {
+ // could throw IllegalArgumentException, but IllegalStateException is
something
+ // that is already expected in a large portion of the codebase.
+ throw new IllegalStateException(
+ "Supplied TServerInstance string: " + val + " does not follow
format: " + FORMAT);
+ }
+ return new TServerInstance(parts[0], parts[1], parts[2]);
+ }
- public TServerInstance(HostAndPort address, String session) {
+ public TServerInstance(HostAndPort address, String session, String group) {
this.hostAndPort = address;
this.session = session;
this.hostPort = hostAndPort.toString();
- this.hostPortSession = hostPort + "[" + session + "]";
+ this.group = group;
+ this.hostPortSessionGroup = String.format(FORMAT, hostPort, session,
group);
}
- public TServerInstance(String formattedString) {
- int pos = formattedString.indexOf("[");
- if (pos < 0 || !formattedString.endsWith("]")) {
- throw new IllegalArgumentException(formattedString);
- }
- this.hostAndPort = HostAndPort.fromString(formattedString.substring(0,
pos));
- this.session = formattedString.substring(pos + 1, formattedString.length()
- 1);
- this.hostPort = hostAndPort.toString();
- this.hostPortSession = hostPort + "[" + session + "]";
+ public TServerInstance(String address, String session, String group) {
+ this(AddressUtil.parseAddress(address, false), session, group);
}
- public TServerInstance(HostAndPort address, long session) {
- this(address, Long.toHexString(session));
+ public TServerInstance(HostAndPort address, long session, String group) {
+ this(address, Long.toHexString(session), group);
}
- public TServerInstance(String address, long session) {
- this(AddressUtil.parseAddress(address, false), Long.toHexString(session));
+ public TServerInstance(String address, long session, String group) {
+ this(AddressUtil.parseAddress(address, false), Long.toHexString(session),
group);
}
- public TServerInstance(Value address, Text session) {
- this(AddressUtil.parseAddress(new String(address.get(), UTF_8), false),
session.toString());
+ public TServerInstance(Value address, Text session, String group) {
+ this(AddressUtil.parseAddress(new String(address.get(), UTF_8), false),
session.toString(),
+ group);
}
@Override
public int compareTo(TServerInstance other) {
if (this == other) {
return 0;
}
- return this.getHostPortSession().compareTo(other.getHostPortSession());
+ return
this.getHostPortSessionGroup().compareTo(other.getHostPortSessionGroup());
}
@Override
public int hashCode() {
- return getHostPortSession().hashCode();
+ final int prime = 31;
Review Comment:
Could method only call `hostPortSessionGroup.hashCode()`?
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientTabletCacheImpl.java:
##########
@@ -686,17 +686,19 @@ private void lookupTablet(ClientContext context, Text
row, boolean retry,
if ((lastEndRow != null) && (ke.prevEndRow() != null)
&& ke.prevEndRow().equals(lastEndRow)) {
locToCache = new CachedTablet(new KeyExtent(ke.tableId(),
ke.endRow(), lastEndRow),
- cachedTablet.getTserverLocation(),
cachedTablet.getTserverSession(),
- cachedTablet.getGoal(), cachedTablet.wasHostingRequested());
+ cachedTablet.getServer(), cachedTablet.getGoal(),
cachedTablet.wasHostingRequested());
} else {
locToCache = cachedTablet;
}
// save endRow for next iteration
lastEndRow = locToCache.getExtent().endRow();
+ log.trace("Caching tablet location: {}", locToCache);
updateCache(locToCache, lcSession);
}
+ } else {
+ log.warn("Metadata tablet not found for row: {}", metadataRow);
Review Comment:
This case could happen when a metadata tablet is temporarily not hosted.
Would not want to warn in that case.
```suggestion
log.debug("Metadata tablet not found for row: {}", metadataRow);
```
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -808,6 +814,11 @@ public enum Property {
+ " global setting to take effect. However, you must use the API or
the shell"
+ " to change properties in zookeeper that are set on a table.",
"1.3.5"),
+ TABLE_ASSIGNMENT_GROUP("table.assignment.group",
ServiceDescriptor.DEFAULT_GROUP_NAME,
+ PropertyType.STRING,
+ "Tablets for this table will be assigned to TabletServers that have a
corresponding"
Review Comment:
This statement may not be true, it depends on the behavior of the balancer?
Unless the balancer is limited to only assigning to tserver within the group?
We could limit balancing choices to a group, so a balancer could only chose
from tservers within this group. That may require some changes to the SPI, not
sure. Or this information could be provided as information to the balancer,
but it does not have to honor it. If the balancer does not have to honor it
then would need to change the description.
##########
core/src/main/java/org/apache/accumulo/core/metadata/TServerInstance.java:
##########
@@ -34,56 +36,74 @@
*/
public class TServerInstance implements Comparable<TServerInstance> {
+ private static final String FORMAT = "%s#%s#%s";
+
private final HostAndPort hostAndPort;
private final String hostPort;
private final String session;
- private final String hostPortSession;
+ private final String hostPortSessionGroup;
+ private final String group;
+
+ public static TServerInstance fromString(String val) {
+ Objects.requireNonNull(val, "String value for TServerInstance cannot be
null");
+ String[] parts = val.split("#");
+ if (parts.length != 3) {
+ // could throw IllegalArgumentException, but IllegalStateException is
something
+ // that is already expected in a large portion of the codebase.
+ throw new IllegalStateException(
+ "Supplied TServerInstance string: " + val + " does not follow
format: " + FORMAT);
+ }
+ return new TServerInstance(parts[0], parts[1], parts[2]);
+ }
- public TServerInstance(HostAndPort address, String session) {
+ public TServerInstance(HostAndPort address, String session, String group) {
this.hostAndPort = address;
this.session = session;
this.hostPort = hostAndPort.toString();
- this.hostPortSession = hostPort + "[" + session + "]";
+ this.group = group;
+ this.hostPortSessionGroup = String.format(FORMAT, hostPort, session,
group);
}
- public TServerInstance(String formattedString) {
- int pos = formattedString.indexOf("[");
- if (pos < 0 || !formattedString.endsWith("]")) {
- throw new IllegalArgumentException(formattedString);
- }
- this.hostAndPort = HostAndPort.fromString(formattedString.substring(0,
pos));
- this.session = formattedString.substring(pos + 1, formattedString.length()
- 1);
- this.hostPort = hostAndPort.toString();
- this.hostPortSession = hostPort + "[" + session + "]";
+ public TServerInstance(String address, String session, String group) {
+ this(AddressUtil.parseAddress(address, false), session, group);
}
- public TServerInstance(HostAndPort address, long session) {
- this(address, Long.toHexString(session));
+ public TServerInstance(HostAndPort address, long session, String group) {
+ this(address, Long.toHexString(session), group);
}
- public TServerInstance(String address, long session) {
- this(AddressUtil.parseAddress(address, false), Long.toHexString(session));
+ public TServerInstance(String address, long session, String group) {
+ this(AddressUtil.parseAddress(address, false), Long.toHexString(session),
group);
}
- public TServerInstance(Value address, Text session) {
- this(AddressUtil.parseAddress(new String(address.get(), UTF_8), false),
session.toString());
+ public TServerInstance(Value address, Text session, String group) {
+ this(AddressUtil.parseAddress(new String(address.get(), UTF_8), false),
session.toString(),
+ group);
}
@Override
public int compareTo(TServerInstance other) {
if (this == other) {
return 0;
}
- return this.getHostPortSession().compareTo(other.getHostPortSession());
+ return
this.getHostPortSessionGroup().compareTo(other.getHostPortSessionGroup());
}
@Override
public int hashCode() {
- return getHostPortSession().hashCode();
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + ((group == null) ? 0 : group.hashCode());
+ result =
+ prime * result + ((hostPortSessionGroup == null) ? 0 :
hostPortSessionGroup.hashCode());
+ return result;
}
@Override
public boolean equals(Object obj) {
+ if (obj == null) {
+ return false;
+ }
Review Comment:
`null instaceof X` should return false, so should not need this check.
```suggestion
```
##########
core/src/main/java/org/apache/accumulo/core/metadata/TServerInstance.java:
##########
@@ -34,56 +36,74 @@
*/
public class TServerInstance implements Comparable<TServerInstance> {
+ private static final String FORMAT = "%s#%s#%s";
+
private final HostAndPort hostAndPort;
private final String hostPort;
private final String session;
- private final String hostPortSession;
+ private final String hostPortSessionGroup;
+ private final String group;
+
+ public static TServerInstance fromString(String val) {
+ Objects.requireNonNull(val, "String value for TServerInstance cannot be
null");
+ String[] parts = val.split("#");
+ if (parts.length != 3) {
+ // could throw IllegalArgumentException, but IllegalStateException is
something
+ // that is already expected in a large portion of the codebase.
+ throw new IllegalStateException(
+ "Supplied TServerInstance string: " + val + " does not follow
format: " + FORMAT);
+ }
+ return new TServerInstance(parts[0], parts[1], parts[2]);
+ }
- public TServerInstance(HostAndPort address, String session) {
+ public TServerInstance(HostAndPort address, String session, String group) {
this.hostAndPort = address;
this.session = session;
this.hostPort = hostAndPort.toString();
- this.hostPortSession = hostPort + "[" + session + "]";
+ this.group = group;
+ this.hostPortSessionGroup = String.format(FORMAT, hostPort, session,
group);
}
- public TServerInstance(String formattedString) {
- int pos = formattedString.indexOf("[");
- if (pos < 0 || !formattedString.endsWith("]")) {
- throw new IllegalArgumentException(formattedString);
- }
- this.hostAndPort = HostAndPort.fromString(formattedString.substring(0,
pos));
- this.session = formattedString.substring(pos + 1, formattedString.length()
- 1);
- this.hostPort = hostAndPort.toString();
- this.hostPortSession = hostPort + "[" + session + "]";
+ public TServerInstance(String address, String session, String group) {
+ this(AddressUtil.parseAddress(address, false), session, group);
}
- public TServerInstance(HostAndPort address, long session) {
- this(address, Long.toHexString(session));
+ public TServerInstance(HostAndPort address, long session, String group) {
+ this(address, Long.toHexString(session), group);
}
- public TServerInstance(String address, long session) {
- this(AddressUtil.parseAddress(address, false), Long.toHexString(session));
+ public TServerInstance(String address, long session, String group) {
+ this(AddressUtil.parseAddress(address, false), Long.toHexString(session),
group);
}
- public TServerInstance(Value address, Text session) {
- this(AddressUtil.parseAddress(new String(address.get(), UTF_8), false),
session.toString());
+ public TServerInstance(Value address, Text session, String group) {
+ this(AddressUtil.parseAddress(new String(address.get(), UTF_8), false),
session.toString(),
+ group);
}
@Override
public int compareTo(TServerInstance other) {
if (this == other) {
return 0;
}
- return this.getHostPortSession().compareTo(other.getHostPortSession());
+ return
this.getHostPortSessionGroup().compareTo(other.getHostPortSessionGroup());
Review Comment:
Is it expected that the group would never change for an instance? If it
did change it would be problematic to include it in compare, hashCode, and
equals methods for tserver instance.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -168,26 +165,6 @@ public TServerInstance getServerInstance() {
return tServerInstance;
}
- public String getHostPortSession() {
- return tServerInstance.getHostPortSession();
- }
-
- public String getHost() {
- return tServerInstance.getHost();
- }
-
- public String getHostPort() {
Review Comment:
Why were these methods removed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
