Avoid metrics persistence setup for in-memory event buffering#4877
Avoid metrics persistence setup for in-memory event buffering#4877mj006648 wants to merge 2 commits into
Conversation
mj006648
force-pushed
the
feature/issue-4594-inmemory-metrics
branch
from
a320d4a to
35dc590
Compare
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
Planning to merge on Jun 26 |
| var metricsPersistence = metaStoreManagerFactory.getOrCreateMetricsPersistence(realmContext); | ||
| var callContext = new PolarisCallContext(realmContext, basePersistence, metricsPersistence); | ||
| var callContext = | ||
| new PolarisCallContext(realmContext, basePersistence, NO_OP_METRICS_PERSISTENCE); |
There was a problem hiding this comment.
hmmm, I feel the right fix should be undo the metric persistence being part of the polaris call context param. But not sure if that's gonna affecting Anand's ongoing work
If we want a quick fix, wonder if the arg takes null value? I'm inclined this approach as it does not introduce the MetricsPersistence class dependency, which its class path will be changed for sure.
| var metricsPersistence = metaStoreManagerFactory.getOrCreateMetricsPersistence(realmContext); | ||
| var callContext = new PolarisCallContext(realmContext, basePersistence, metricsPersistence); | ||
| var callContext = | ||
| new PolarisCallContext(realmContext, basePersistence, NO_OP_METRICS_PERSISTENCE); |
There was a problem hiding this comment.
I think this is a side effect of separating metricsPersistence, which forces PolarisCallContext to carry more xxxPersistence that is not needed. I don't like that direction, but since we are here. I'd create a new PolarisCallContext constructor to avoid something like NO_OP_METRICS_PERSISTENCE. If u look at the class, we actually have that constructor. Maybe we just need to remove the depreaction annotation.
Move the no-op fallback behind PolarisCallContext so the in-memory listener only needs BasePersistence. Constraint: Review feedback asked to avoid listener-local MetricsPersistence Confidence: high Scope-risk: narrow Tested: targeted listener test; format compileAll Not-tested: runtime-service:check hits pre-existing NoSQL/InMem bootstrap failure locally
|
Thanks @flyrain and @flyingImer for the pointers. I updated this to remove the MetricsPersistence dependency from InMemoryBufferEventListener. The listener now just passes the BasePersistence it already needs, and the fallback stays inside PolarisCallContext through the convenience constructor. CI is green now. |
|
@mj006648 : sorry to cause conflicts here... Could you rebase, please? |
5 participants
Summary
Fixes #4594.
This avoids creating MetricsPersistence for InMemoryBufferEventListener. The listener only needs to write buffered event entities, so it now passes a no-op MetricsPersistence into PolarisCallContext instead of asking the MetaStoreManagerFactory to create one.
The focused buffer-size test now verifies that flushing events does not request metrics persistence.
Tests
./gradlew :polaris-runtime-service:test --tests "org.apache.polaris.service.events.listeners.inmemory.InMemoryBufferEventListenerBufferSizeTest"./gradlew format./gradlew compileAllChecklist
CHANGELOG.md, if neededsite/content/in-dev/unreleased, if needed