markap14 commented on a change in pull request #4745:
URL: https://github.com/apache/nifi/pull/4745#discussion_r556586477
##########
File path:
nifi-commons/nifi-site-to-site-client/src/main/java/org/apache/nifi/remote/client/AbstractPeerPersistence.java
##########
@@ -44,11 +44,19 @@ protected PeerStatusCache restorePeerStatuses(final
BufferedReader reader,
return null;
}
+ final String remoteInstanceUris;
+ remoteInstanceUris = String.valueOf(reader.readLine());
+ if (remoteInstanceUris == null ||
!remoteInstanceUris.matches("(.+)://(.+):(.+)/nifi-api(.*)")) {
Review comment:
I don't think there's any reason to run a regular expression against the
URIs - just allow whatever the user entered. Also, that regular expression is
not accurate because it assumes that a port must be given, and it may not be.
There is also no need to include the `/nifi-api` at the end - the URL that the
user enters may be as simple as something like `https://nifi-01`. So I would
avoid checking anything like that. Instead, I would suggest that when writing
out the line, we write out something like "Remote Instance URI: <uris>". Then
when reading it in, we can check if the line starts with "Remote Instance URI:
". And/or I would suggest that the second line should actually be something
along the lines of "Encoding-Version: 2". That would make it much simpler in
the future to modify this code, by simply reading in the Encoding Version and
then we know what to expect.
##########
File path:
nifi-commons/nifi-site-to-site-client/src/main/java/org/apache/nifi/remote/client/AbstractPeerPersistence.java
##########
@@ -44,11 +44,19 @@ protected PeerStatusCache restorePeerStatuses(final
BufferedReader reader,
return null;
}
+ final String remoteInstanceUris;
+ remoteInstanceUris = String.valueOf(reader.readLine());
Review comment:
No need to call String.valueOf here, or the declare `remoteInstanceUris`
separately. Can just be `final String remoteInstanceUris = reader.readLine();`
##########
File path:
nifi-commons/nifi-site-to-site-client/src/test/groovy/org/apache/nifi/remote/client/PeerSelectorTest.groovy
##########
@@ -71,6 +72,11 @@ class PeerSelectorTest extends GroovyTestCase {
}
+ private static String buildRemoteInstanceUris(List<String> nodes =
DEFAULT_NODES) {
+ String remoteInstanceUris = new
String("http://";).concat(nodes.join(":8443/nifi-api,http://";)).concat(":8443/nifi-api");
Review comment:
Should just use `String remoteInstanceUris = "http://"; +
nodes.join(":8443/nifi-api,http://"; + ":8443/nifi-api");`... no need to use new
String() or concat methods.
##########
File path:
nifi-commons/nifi-site-to-site-client/src/main/java/org/apache/nifi/remote/client/PeerSelector.java
##########
@@ -106,6 +106,16 @@ private void restoreInitialPeerStatusCache() {
final SiteToSiteTransportProtocol currentProtocol =
peerStatusProvider.getTransportProtocol();
final SiteToSiteTransportProtocol cachedProtocol =
restoredPeerStatusCache.getTransportProtocol();
+ final String currentRemoteInstanceUris =
peerStatusProvider.getRemoteInstanceUris();
+ final String cachedRemoteInstanceUris =
restoredPeerStatusCache.getRemoteInstanceUris();
+
+ // If the remote instance URIs have changed, clear the
cache
+ if
(!currentRemoteInstanceUris.equals(cachedRemoteInstanceUris)) {
+ logger.warn("Discard stored peer statuses in {}
because remote instance URIs has changed from {} to {}",
Review comment:
No reason to warn here. This is a normal and expected thing. Can use
INFO or really even DEBUG.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
