perf(server): put getables for a single wfrun under same prefix #1842
Conversation
|
I need to do some work on the |
coltmcnealy-lh
force-pushed
the
perf/index-locality
branch
from
606ae83 to
d8a6a32
Compare
This backwards-compatible change puts all getable's such as TaskRun's and NodeRun's for a single WfRun onto the same prefix. Doing this will put most of the keys for a WfRun onto the same block in RocksDB (or in adjacent blocks), which optimizes I/O both in compaction and in cold reads.
coltmcnealy-lh
force-pushed
the
perf/index-locality
branch
from
9a7a2a0 to
f34cc35
Compare
…itable for testing the canary
|
Setup:
This is the Disk I/O when retention starts with this change: THis is the Disk I/O when retention starts before this change: |
|
Canary tail latencies also seem to improve a little bit but it's slight. Here's the before: And here's the after: |
…key format handling in ReadOnlyBaseStoreImpl
…elete objects in new and old formats at the same time when there is a grouped key
…use it for key generation, for listing stored getables in the new format
…y; update getRocksDBKey to use the new method
… up imports in SearchWfRunRequestModel
|
@l4crito Great work! I tested your changes since I left it off and it all works. |
coltmcnealy-lh
left a comment
coltmcnealy-lh
left a comment
There was a problem hiding this comment.
Approved. But technically I opened this PR so I can't approve it myself.
examples/java/external-event/src/main/java/io/littlehorse/examples/ExternalEventExample.java
Show resolved
Hide resolved
server/src/main/java/io/littlehorse/common/model/getable/GroupedObjectId.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/littlehorse/common/model/getable/objectId/TaskRunIdModel.java
Show resolved
Hide resolved
server/src/main/java/io/littlehorse/server/monitoring/HealthService.java
Outdated
Show resolved
Hide resolved
|
|
||
| String endKey = req.boundedObjectIdScan.getEndObjectId() + "~"; | ||
| String endKey = | ||
| StoredGetable.getRocksDBKey(req.boundedObjectIdScan.getStartObjectId(), req.getObjectType()) + "~"; |
There was a problem hiding this comment.
Is there a reason why we are using getStartObjectId() instead of getEndObjectId()? It might work but still it seems not so kosher.
There was a problem hiding this comment.
there was a reason but i don't remember exactly what it was, let me debug really quick
There was a problem hiding this comment.
I think that getEndObjectId() will return something like wfrunId~ and when we build the prefix from it, it is gonna be /tenant/storable/wrg/wfrunId~/storable/gettable
There was a problem hiding this comment.
Can you create a follow-up ticket with type Refactor to address this? We should properly refactor the Internal Scan protobuf to understand the concept of grouping WfRun's. I think that would be a cleaner implementation; but it can wait for a while longer I think.
server/src/main/java/io/littlehorse/server/streams/store/StoredGetable.java
Show resolved
Hide resolved
There was a problem hiding this comment.
A necessary sacrifice 🫡
| return getFullStoreKey(getType(), getStoreKey()); | ||
| if (getGroupingWfRunId().isPresent()) { | ||
| if (!(this instanceof StoredGetable)) { | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
I'm still confused, how does this work with WfRunStoredInventoryModel? Are those objects stored in a different key prefix?
This backwards-compatible change puts all getable's such as TaskRun's and NodeRun's for a single WfRun onto the same prefix. Doing this will put most of the keys for a WfRun onto the same block in RocksDB (or in adjacent blocks), which optimizes I/O both in compaction and in cold reads.