DrDread746 commented on PR #7727: URL: https://github.com/apache/shenyu/pull/7727#issuecomment-6246540484
Thank you for the thorough and invaluable review! I have updated the implementation to address all your points in the latest commit: 1. clientsMap Leak: Added an isCompleted.get() guard at the top of run() before queueing, and ensured cleanUp() is unconditionally/idempotently invoked so stale clients cannot stay stuck in clientsMap. 2. Configurable Cadence: Exposed heartbeatInterval in HttpSyncProperties (defaulting to 10s) so liveness reporting is tunable via configuration. 3. Scheduler Isolation: Created a dedicated livenessScheduler (shenyu-http-long-polling-liveness-) with setRemoveOnCancelPolicy(true) and @PreDestroy container cleanup to decouple liveness event publishing from long-polling task execution. 4. Blank Port Filtering: Updated shouldReportInstance() to reject null, blank, or whitespace-only ports alongside "0". 5. Test Refactoring: Removed reflection in favor of package-private accessors, replaced wall-clock Thread.sleep assertions with scheduled future assertions, and renamed the reproduction test to documentation_reproduceFalseOfflineTimeoutBeforeFix. 6. Design Rationale Note: Noted in the PR description that refreshing during long-poll holds preserves zero runtime overhead on InstanceCheckService without altering global health check logic. -- 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]
