LuoYushi7 commented on issue #4755:
URL:
https://github.com/apache/rocketmq-dashboard/issues/4755#issuecomment-5760036412
I would like to take this one if you are not already on it.
Reading the spec, the part I want to confirm before writing code is the
retry rule in
step 4, because it is the only piece that changes observable control flow:
> If it races with a retired cache entry, remove that exact lease and retry
lookup so
> the request obtains a current client.
Bounded or unbounded? A single retry covers the ordinary case (the entry we
grabbed was
just retired by a concurrent endpoint update). A loop instead survives a
burst of
successive updates but can spin if retirement keeps winning. I lean towards
one retry
returning the same `CLIENT_NOT_FOUND`/`INSTANCE_NOT_FOUND` mapping as a
plain cache miss
when the second lookup also comes back retired, so the caller sees a normal
failure rather
than a silent extra wait.
Two smaller ones:
* Is `ClientLease` shared between `MqAdminExtFactory` and `MqClientPool`, or
one per pool?
The acceptance criteria say shutdown is invoked at most once per cached
client, which a
single lease per entry gives for free — I read it as shared, in its own
file.
* For "All new test method names end with `Test`" — that matches the
existing convention
in the metrics/collector tests, so I will follow it.
If you are already implementing this, say so and I will leave it alone
rather than add a
duplicate to the review queue.
--
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]
