From 83fc33af8996a8422d4fb6579dbf74aa0212c29a Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Tue, 12 Nov 2024 16:38:10 +0100 Subject: [PATCH] app/vmauth: simplify the logic for the fix at a0a154511a1f26035cd714987fc2924e70087f96 The fix at a0a154511a1f26035cd714987fc2924e70087f96 looks too complicated and fragile: - It moves buMin initialization to the place, which is far from its usage. - It embeds unclear logic on selecting the proper buMin if it is broken, into unrelated loop. The actual fix must be more clear: $ git diff 95acca6b5223c8e92c82935183cf1159b9046196 -- app/vmauth/ - if n := bu.concurrentRequests.Load(); n < minRequests { + if n := bu.concurrentRequests.Load(); n < minRequests || buMin.isBroken() { This should simplify further maintenance of this code. Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/7489 Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3061 --- app/vmauth/auth_config.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/app/vmauth/auth_config.go b/app/vmauth/auth_config.go index 018dce6ba..63f177ef5 100644 --- a/app/vmauth/auth_config.go +++ b/app/vmauth/auth_config.go @@ -462,17 +462,12 @@ func getLeastLoadedBackendURL(bus []*backendURL, atomicCounter *atomic.Uint32) * // Slow path - select other backend urls. n := atomicCounter.Add(1) - 1 - buMin := bus[n%uint32(len(bus))] for i := uint32(0); i < uint32(len(bus)); i++ { idx := (n + i) % uint32(len(bus)) bu := bus[idx] if bu.isBroken() { continue } - if buMin.isBroken() { - // verify that buMin isn't set as broken - buMin = bu - } if bu.concurrentRequests.Load() == 0 { // Fast path - return the backend with zero concurrently executed requests. // Do not use CompareAndSwap() instead of Load(), since it is much slower on systems with many CPU cores. @@ -482,12 +477,13 @@ func getLeastLoadedBackendURL(bus []*backendURL, atomicCounter *atomic.Uint32) * } // Slow path - return the backend with the minimum number of concurrently executed requests. + buMin := bus[n%uint32(len(bus))] minRequests := buMin.concurrentRequests.Load() for _, bu := range bus { if bu.isBroken() { continue } - if n := bu.concurrentRequests.Load(); n < minRequests { + if n := bu.concurrentRequests.Load(); n < minRequests || buMin.isBroken() { buMin = bu minRequests = n }