Cancel xds stream ready timeout when the stream is closed. Correct xds connection status metric. #1007
Conversation
…s connection status metric.
bohhyang
requested review from
PapaCharlie,
nizarm,
ZoabKapoor,
shivamgupta1,
lauraliu61b and
brycezhongqing
d2/src/main/java/com/linkedin/d2/xds/XdsToD2PropertiesAdaptor.java
Outdated
Show resolved
Hide resolved
d2/src/main/java/com/linkedin/d2/xds/XdsToD2PropertiesAdaptor.java
Outdated
Show resolved
Hide resolved
| if (_retryRpcStreamFuture != null) | ||
| { | ||
| _log.info("ADS stream reconnected after {} milliseconds", | ||
| _retryRpcStreamFuture.getDelay(TimeUnit.MILLISECONDS)); | ||
| _retryRpcStreamFuture = null; | ||
| _xdsClientJmx.incrementReconnectionCount(); |
There was a problem hiding this comment.
All these conditions seem to be precluded by the isInBackoff check above at line 255? Also, getDelay seems to return the still remaining delay and not the elapsed time.
There was a problem hiding this comment.
precluded by the isInBackoff check above at line 255
not really. This is not for checking backoff but just to clean _retryRpcStreamFuture for the case when the retry future is executed (and the reconnect stream is ready), but the future itself is not cleaned. This check will differentiate the initial connect from reconnections (previously, reconnectionCount metric was incremented for initial connection as well, which was not a reconnect).
getDelay seems to return the still remaining delay
whoops, that's right. Removed, thanks.
| // Also confirm ready timeout future is not null to avoid notifying multiple times. | ||
| if (!_adsStream.isReady() || _readyTimeoutFuture == null) | ||
| { | ||
| // if the ready timeout future is non-null, a reconnect notification hasn't been sent yet. | ||
| if (_readyTimeoutFuture != null) | ||
| { | ||
| // timeout task will be cancelled only if it hasn't already executed. | ||
| boolean cancelledTimeout = _readyTimeoutFuture.cancel(false); | ||
| _log.info("ADS stream ready, cancelled timeout task: {}", cancelledTimeout); | ||
| _readyTimeoutFuture = null; // set it to null to avoid repeat notifications to subscribers. | ||
| _xdsClientJmx.incrementReconnectionCount(); | ||
| notifyStreamReconnect(); | ||
| } | ||
| _xdsClientJmx.setIsConnected(true); | ||
| return; | ||
| } | ||
|
|
||
| // timeout task will be cancelled only if it hasn't already executed. | ||
| boolean cancelledTimeout = _readyTimeoutFuture.cancel(false); | ||
| _log.info("ADS stream ready, cancelled timeout task: {}", cancelledTimeout); | ||
| _readyTimeoutFuture = null; // set it to null to avoid repeat notifications to subscribers. |
There was a problem hiding this comment.
Although not too bad, this might be some unneeded refactoring? (don't fix what ain't broken)
There was a problem hiding this comment.
yeah, it's just return-early, as suggested by @brycezhongqing above.
Summary
See context at slack discussion.
This change:
Tests
N/A