rzo1 commented on code in PR #260:
URL: https://github.com/apache/openjpa/pull/260#discussion_r4458967958
##########
openjpa-kernel/src/main/java/org/apache/openjpa/event/TCPRemoteCommitProvider.java:
##########
@@ -236,40 +261,18 @@ public void setAddresses(final String names) throws
UnknownHostException {
String[] toks = StringUtil.split(names, ";", 0);
_addresses = new ArrayList<>(toks.length);
- InetAddress localhost = InetAddress.getLocalHost();
- String localhostName = localhost.getHostName();
-
for (String host : toks) {
- String hostname;
- int tmpPort;
- int colon = host.indexOf(':');
- if (colon != -1) {
- hostname = host.substring(0, colon);
- tmpPort = Integer.parseInt(host.substring(colon + 1));
- } else {
- hostname = host;
- tmpPort = DEFAULT_PORT;
- }
- InetAddress tmpAddress = InetAddress.getByName(hostname);
-
- // bleair: For each address we would rather make use of
- // the jdk1.4 isLinkLocalAddress () || isLoopbackAddress ().
- // (Though in practice on win32 they don't work anyways!)
- // Instead we will check hostname. Not perfect, but
- // it will match often enough (people will typically
- // use the DNS machine names and be cutting/pasting.)
- if (localhostName.equals(hostname)) {
+ HostAddress newAddress = new HostAddress(host);
+ if (localAddresses.contains(newAddress._address)) {
Review Comment:
This drops every peer on the same host, whatever its port. So `Port=5636,
Addresses=127.0.0.1:6636` (two providers on one machine) ends up with no peers.
I checked it against the built classes: master keeps the peer for
`127.0.0.1:6636`, `localhost:6636` and the host's own IP, this branch keeps
none. Only skip it if the port is ours as well, like `fromSelf` does on the
receiving side:
```suggestion
if (newAddress._port == _port &&
localAddresses.contains(newAddress._address)) {
```
This relies on `Port` being set before `Addresses`. If the order isn't
guaranteed, the filter could move to `endConfiguration()`.
##########
openjpa-kernel/src/main/resources/org/apache/openjpa/event/localizer.properties:
##########
@@ -80,6 +80,7 @@ tcp-received-connection: Received new connection from "{0}"
tcp-start-listener: Started listening for remote commit information on \
port "{0}".
tcp-address-asself: Identified address of "{0}", which is equal to ourself.
+tcp-address-notself: Identified address of "{0}", doesn't belongs to known
local addresses.
Review Comment:
Small wording fix:
```suggestion
tcp-address-notself: The bind address "{0}" is not an address of this host.
```
##########
openjpa-kernel/src/main/java/org/apache/openjpa/event/TCPRemoteCommitProvider.java:
##########
@@ -94,18 +96,41 @@ public class TCPRemoteCommitProvider
protected List<HostAddress> _addresses = new ArrayList<>();
protected final ReentrantLock _addressesLock;
- public TCPRemoteCommitProvider() throws UnknownHostException {
+ public TCPRemoteCommitProvider() throws UnknownHostException,
SocketException {
// obtain a unique ID.
synchronized (TCPRemoteCommitProvider.class) {
_id = s_idSequence++;
}
- // cache the local IP address.
+ // cache the local IP addresses.
+ localAddresses = NetworkInterface.networkInterfaces()
+ .flatMap(NetworkInterface::inetAddresses)
+ .collect(Collectors.toUnmodifiableSet());
+
+ // cache our own IP address.
_localhost = InetAddress.getLocalHost().getAddress();
+
_addressesLock = new ReentrantLock();
setNumBroadcastThreads(2);
}
+ /**
+ * Set address this provider should listen on. Set once only.
+ *
+ * @param address address this provider should listen on
+ */
+ public void setBindAddress(final String address) throws
UnknownHostException {
+ InetAddress addr = InetAddress.getByName(address);
+ if (!localAddresses.contains(addr)) {
+ if (log.isWarnEnabled()) {
+ log.warn(s_loc.get("tcp-address-notself", addr));
+ }
+ return;
+ }
Review Comment:
If the address is not local, this only logs a warning, and the provider then
listens on all interfaces. That's the opposite of what was configured. I'd
rather fail here:
```suggestion
if (!localAddresses.contains(addr)) {
throw new UserException(s_loc.get("tcp-address-notself", addr));
}
```
(needs `import org.apache.openjpa.util.UserException;`)
##########
openjpa-kernel/src/main/java/org/apache/openjpa/event/TCPRemoteCommitProvider.java:
##########
@@ -293,12 +296,11 @@ public void endConfiguration() {
if (_listener == null || (!_listener.isRunning() &&
_listener._port == _port)) {
try {
- _listener = new TCPPortListener(_port, log);
+ _listener = new TCPPortListener(_port, _bindAddress, log);
Review Comment:
The listeners are still shared per port only. A second provider in the same
JVM with the same port but another `BindAddress` silently reuses the first
listener, and with it the first one's address. Maybe key the map by bind
address and port, or at least warn when the addresses differ.
##########
openjpa-kernel/src/main/java/org/apache/openjpa/event/TCPRemoteCommitProvider.java:
##########
@@ -94,18 +96,41 @@ public class TCPRemoteCommitProvider
protected List<HostAddress> _addresses = new ArrayList<>();
protected final ReentrantLock _addressesLock;
- public TCPRemoteCommitProvider() throws UnknownHostException {
+ public TCPRemoteCommitProvider() throws UnknownHostException,
SocketException {
Review Comment:
The constructor now declares `SocketException`. Subclasses outside OpenJPA
that declare only `UnknownHostException` no longer compile. Not a big deal, but
you could wrap it, or mention it in the migration notes.
--
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]
