kevinrr888 commented on code in PR #5662:
URL: https://github.com/apache/accumulo/pull/5662#discussion_r2161789097
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1123,14 +1121,14 @@ public void run() {
managerClientHandler = new ManagerClientServiceHandler(this);
compactionCoordinator = new CompactionCoordinator(this, fateRefs);
- ServerAddress sa;
var processor = ThriftProcessorTypes.getManagerTProcessor(this,
fateServiceHandler,
compactionCoordinator.getThriftService(), managerClientHandler,
getContext());
-
try {
- sa = TServerUtils.createThriftServer(context, getBindAddress(),
Property.MANAGER_CLIENTPORT,
- processor, "Manager", null, Property.MANAGER_MINTHREADS,
- Property.MANAGER_MINTHREADS_TIMEOUT, Property.MANAGER_THREADCHECK);
+ startThriftServer(() -> {
+ return TServerUtils.createThriftServer(context, getBindAddress(),
+ Property.MANAGER_CLIENTPORT, processor, "Manager", null,
Property.MANAGER_MINTHREADS,
+ Property.MANAGER_MINTHREADS_TIMEOUT, Property.MANAGER_THREADCHECK);
+ }, false);
Review Comment:
It doesn't seem correct to update the advertise address here. Before it was
needed to be set after creating the lock
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -343,7 +330,7 @@ private ServiceLock announceExistence() {
for (ThriftService svc : new ThriftService[] {ThriftService.CLIENT,
ThriftService.TABLET_SCAN}) {
descriptors.addService(new ServiceDescriptor(serverLockUUID, svc,
- getClientAddressString(), this.getResourceGroup()));
+ getAdvertiseAddress().toString(), this.getResourceGroup()));
Review Comment:
```suggestion
String.valueOf(getAdvertiseAddress()),
this.getResourceGroup()));
```
Was handling null before, not sure if it can be null, but this would handle
that
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java:
##########
@@ -475,10 +469,7 @@ private ServerAddress startTabletClientService() throws
UnknownHostException {
TProcessor processor =
ThriftProcessorTypes.getTabletServerTProcessor(this, clientHandler,
thriftClientHandler,
scanClientHandler, thriftClientHandler, thriftClientHandler,
getContext());
- ServerAddress address = startServer(getBindAddress().toString(),
processor);
- updateAdvertiseAddress(address.address);
- log.info("address = {}", address);
- return address;
+ startServer(getBindAddress().toString(), processor);
Review Comment:
```suggestion
startServer(getBindAddress(), processor);
```
already a string
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java:
##########
@@ -207,8 +206,6 @@ public PausedCompactionMetrics getPausedCompactionMetrics()
{
private final BlockingDeque<ManagerMessage> managerMessages = new
LinkedBlockingDeque<>();
- volatile HostAndPort clientAddress;
-
private ServiceLock tabletServerLock;
private TServer server;
Review Comment:
`server` is never assigned. Should be removed along with its usages
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java:
##########
@@ -281,7 +281,7 @@ private synchronized void startLogMaker() {
try {
alog = DfsLogger.createNew(tserver.getContext(), syncCounter,
flushCounter,
- tserver.getClientAddressString());
+ tserver.getAdvertiseAddress().toString());
Review Comment:
```suggestion
String.valueOf(tserver.getAdvertiseAddress()));
```
##########
server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java:
##########
@@ -302,6 +309,17 @@ public String getBindAddress() {
return bindAddress;
}
+ protected TServer getThriftServer() {
+ if (thriftServer == null) {
+ return null;
+ }
+ return thriftServer.server;
+ }
+
+ protected ServerAddress getThriftServerAddress() {
+ return thriftServer;
+ }
+
protected void updateAdvertiseAddress(HostAndPort thriftBindAddress) {
Review Comment:
`updateAdvertiseAddress()` is not thread safe using volatile for
advertiseAddress (checking advertiseAddress then writing to it).
Not part of this PR so could be fixed in follow on.
Would also be good to ensure all uses of thriftServer and advertiseAddress
are correct. May want to consider using atomic reference for these
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java:
##########
@@ -608,6 +596,7 @@ public void run() {
});
HostAndPort managerHost;
+ final String advertiseAddressString = getAdvertiseAddress().toString();
Review Comment:
```suggestion
final String advertiseAddressString =
String.valueOf(getAdvertiseAddress());
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java:
##########
@@ -772,15 +761,8 @@ public void run() {
}
}
- public String getClientAddressString() {
- if (clientAddress == null) {
- return null;
- }
- return clientAddress.getHost() + ":" + clientAddress.getPort();
- }
-
public TServerInstance getTabletSession() {
- String address = getClientAddressString();
+ String address = getAdvertiseAddress().toString();
if (address == null) {
Review Comment:
address cannot be null here. Probably want .equals("null") or check if
getAdvertiseAddress() == null at the start of the method (I prefer the latter)
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java:
##########
@@ -901,7 +881,7 @@ public TabletServerStatus
getStats(Map<TableId,MapCounter<ScanRunState>> scanCou
result.lastContact = RelativeTime.currentTimeMillis();
result.tableMap = tables;
result.osLoad =
ManagementFactory.getOperatingSystemMXBean().getSystemLoadAverage();
- result.name = getClientAddressString();
+ result.name = getAdvertiseAddress().toString();
Review Comment:
```suggestion
result.name = String.valueOf(getAdvertiseAddress());
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java:
##########
@@ -772,15 +761,8 @@ public void run() {
}
}
- public String getClientAddressString() {
- if (clientAddress == null) {
- return null;
- }
- return clientAddress.getHost() + ":" + clientAddress.getPort();
- }
-
public TServerInstance getTabletSession() {
- String address = getClientAddressString();
+ String address = getAdvertiseAddress().toString();
Review Comment:
```suggestion
String address = String.valueOf(getAdvertiseAddress());
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1151,11 +1149,6 @@ public void run() {
// starting the upgrade process if necessary.
setManagerState(ManagerState.HAVE_LOCK);
- // Set the HostName **after** initially creating the lock. The lock data is
- // updated below with the correct address. This prevents clients from
accessing
- // the Manager until all of the internal processes are started.
- updateAdvertiseAddress(sa.getAddress());
-
Review Comment:
Referring to this
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java:
##########
@@ -507,7 +498,7 @@ private void announceExistence() {
ThriftService.TABLET_INGEST, ThriftService.TABLET_MANAGEMENT,
ThriftService.TABLET_SCAN,
ThriftService.TSERV}) {
descriptors.addService(new ServiceDescriptor(tabletServerUUID, svc,
- getClientAddressString(), this.getResourceGroup()));
+ getAdvertiseAddress().toString(), this.getResourceGroup()));
Review Comment:
```suggestion
String.valueOf(getAdvertiseAddress()),
this.getResourceGroup()));
```
##########
server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java:
##########
@@ -311,6 +329,17 @@ protected void updateAdvertiseAddress(HostAndPort
thriftBindAddress) {
}
}
+ protected void startThriftServer(ThriftServerSupplier supplier, boolean
start)
+ throws UnknownHostException {
+ thriftServer = supplier.get();
+ if (start) {
+ thriftServer.startThriftServer("Thrift Client Server");
+ log.debug("Starting {} listening on {}", this.getClass().getSimpleName(),
+ thriftServer.address);
Review Comment:
Would this be better for info level?
--
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]
