MantisMasterClientApi: Added connection caching for discovery informa… #28
Conversation
…tion and status information.
codyrioux
requested review from
jeffchao,
neerajrj,
nickmahilani,
piygoyal and
skalidindi
as code owners
| .onErrorResumeNext(Observable.empty()) | ||
| .doOnCompleted(() -> { | ||
| logger.info("Purging {} from job status cache", id); | ||
| jobSchedulingInfoCache.remove(id); |
There was a problem hiding this comment.
@codyrioux This change will affect mantisclient and hence all jobs as well. Should we move the caching up a layer to reduce potential impact?
There was a problem hiding this comment.
Up a layer as in directly inside Mantis API?
If so, already implemented. I thought we might want to do it here because multiple connections to jobs via the Mantis API (if they use different query params) will still cause multiple subscriptions to scheduling info.
There was a problem hiding this comment.
I see, yah in that case we could make this optional and default it to false, and then only enable it on mantis api first and then slowly roll out to jobs perhaps?
There was a problem hiding this comment.
yeah, given the blast radius, +1 for putting this behind a property so it can be disabled at runtime if we see any issues
There was a problem hiding this comment.
+1 on rollout. should we also add a unit test to test this behavior?
| logger.info("Purging {} from job status cache", id); | ||
| jobSchedulingInfoCache.remove(id); | ||
| }) | ||
| .replay(25) |
There was a problem hiding this comment.
missing a doOnError to clear the cache? Wonder if that is causing the log line to not get printed in your testing.
There was a problem hiding this comment.
Interestingly the cache is evicted immediately with the new doOnError, but still no error message logged.
| .retryWhen(retryLogic) | ||
| ; | ||
| .doOnCompleted(() -> { | ||
| logger.info("Purging {} from job scheudling info cache", id); |
There was a problem hiding this comment.
typo: scheudling -> scheduling
codyrioux
force-pushed
the
connection-caching
branch
from
98d8eaf to
84eefa3
Compare
| logger.info("Purging {} from job status cache", id); | ||
| jobSchedulingInfoCache.remove(id); | ||
| }) | ||
| .doOnError((t) -> { |
There was a problem hiding this comment.
noticed the onErrorResumeNext() up this observable chain, so this doOnError will not get executed, it should probably be before the onErrorResumeNext
Information and status
Added caching similar to MantisAPI to reduce the overall connections any one client maintains to Mantis Master.
Note: I'm not seeing the "Purging jobId from job scheduling info cache" messages for whatever reason but it does appear to be correctly reusing the connection. This is evident as additional incoming connections do not showing additional "Caching scheduling change observable for jobId." log line. Furthermore if all connections are closed and then another re-opened we do see the "Caching scheduling change observable for jobId."
I have the release candidate built off this pulled into a local
mantis-apibuild which has its own connection re-use turned off. Everything is operating as expected.Context
This pull request causes the client to re-use existing scheduling information and status update streams from the master thus reducing the overall load on Mantis Master.
Checklist
./gradlew buildcompiles code correctly./gradlew testpasses all testsCONTRIBUTING.md