An approach to fixing redundant dbserver queries. #92
Conversation
| */ | ||
| private final Map<Integer, Integer> dbServerPorts = new ConcurrentHashMap<>(); | ||
| private final Map<InetAddress, Integer> dbServerPorts = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
This is the main change I mentioned on Zulip. We now track database servers by IP address, rather than by device number, because that is how they are actually partitioned.
| @@ -244,7 +251,12 @@ public void deviceFound(final DeviceAnnouncement announcement) { | |||
| return; | |||
| } | |||
| logger.debug("Processing device found, number: {}, name: {}", announcement.getDeviceNumber(), announcement.getDeviceName()); | |||
| new Thread(() -> requestPlayerDBServerPort(announcement)).start(); | |||
| final Thread queryThread = new Thread(() -> requestPlayerDBServerPort(announcement)); | |||
There was a problem hiding this comment.
We do need a small bit more protection against the fact that we are going to see multiple devices with the same IP address in short succession. The simplest way I could think of to do that is to keep track of which addresses we are already querying for, and not starting a new thread if one is already working. The putIfAbsent() method of ConcurrentHashMap is a lightweight but atomic way to ensure that. If it returns a null value, the new thread was added to the map, so we should start it. Otherwise, we just discard the thread unused, because we know another is working on that address.
The running thread will remove itself as it ends, see balow.
| } | ||
| }; | ||
|
|
||
|
|
There was a problem hiding this comment.
I realized that this next method was only used once, is private, and does so little that I should just get rid of it.
| // logger.info("Trying to determine database server port for device number " + announcement.getNumber()); | ||
| for (int tries = 0; tries < 4; ++tries) { | ||
| try { | ||
| logger.debug("Trying to determine database server port for device number {}", announcement.getDeviceNumber()); |
There was a problem hiding this comment.
Rather than commenting out the log entry, I just dropped it to DEBUG level, so it normally does nothing, but can be seen if I want debug-level output.
| if (tries > 0) { | ||
| try { | ||
| Thread.sleep(1000 * tries); // Give the player more time to be ready, it may be booting. | ||
| } catch (InterruptedException e) { |
There was a problem hiding this comment.
Previously this exception would never occur, but it now actually is expected, if Beat Link shuts down while the thread is sleeping. (You will see the code below that causes that.) We now treat it as a signal to stop trying for the port.
| if (portReturned == 65535) { | ||
| logger.info("Player {} reported dbserver port of {}, not yet ready?", announcement.getDeviceNumber(), portReturned); | ||
| } else { | ||
| if (isRunning()) { // Bail if we were shut down before we received a response. |
There was a problem hiding this comment.
If the thread is not sleeping when we tried to interrupt it, we can still reach this point, so we do one final check to make sure Beat Link did not shut down before we record the port number.
The rest of the changes in this section are just indentation, so don’t strain your eyes trying to see what changed! 😅
| announcement.getDeviceNumber()); | ||
| } catch (Throwable t) { | ||
| logger.error("Problem querying for database server port on device {}:", announcement.getDeviceNumber(), t); | ||
| } finally { |
There was a problem hiding this comment.
This new finally clause is critical. It is here that we remove the thread from the map of addresses to active threads querying them; no matter how the thread ends, via normal return, early return, or exception, this code will run, and remove it from that map, so we can know it is no longer active.
| @@ -522,6 +540,9 @@ public synchronized void stop() { | |||
| if (isRunning()) { | |||
| running.set(false); | |||
| DeviceFinder.getInstance().removeDeviceAnnouncementListener(announcementListener); | |||
| for (Thread thread : activeQueryThreads.values()) { | |||
There was a problem hiding this comment.
And finally here is the new code to interrupt any threads that are querying for DB server ports when we are told to shut down.
@Kevinnns this is the smaller, more focused change I was thinking of. I think it will work;, let’s look it over together before I merge it, and then we can test it on your AZ?