Optimized partition metadata lookup #4443
Conversation
mmaslankaprv
force-pushed
the
limit-metadata-copies
branch
from
2c898c2
to
2d7406a
Compare
travisdowns
force-pushed
the
limit-metadata-copies
branch
from
ccf8cf9
to
2d7406a
Compare
|
@mmaslankaprv when you say fixes the issue can you do a simple |
mmaslankaprv
force-pushed
the
limit-metadata-copies
branch
from
2d7406a
to
7c59fc9
Compare
Optimized per partition metadata lookup by storing partition metadata in `absl::btree_set` the set provides fast lookup while keeping partitions ordered. Limited number of times partition metadata are copied by returning metadata object stored directly in a map. Signed-off-by: Michal Maslanka <[email protected]>
mmaslankaprv
force-pushed
the
limit-metadata-copies
branch
from
7c59fc9
to
33270b9
Compare
Introduced method returning reference to topic metadata. The reference returning method allow us to avoid copying metadata object when it has to be accessed by synchronous parts of code. Important: Remember not to use the reference when its lifetime has to span across multiple scheduling point. Reference returning method is provided not to copy metadata object when used by synchronous parts of code Signed-off-by: Michal Maslanka <[email protected]>
mmaslankaprv
force-pushed
the
limit-metadata-copies
branch
from
33270b9
to
d32572c
Compare
mmaslankaprv
requested review from
rystsov,
NyaliaLui,
graphcareful,
VadimPlh and
ztlpn
as code owners
| /// span across multiple scheduling point. Reference returning method is | ||
| /// provided not to copy metadata object when used by synchronous parts of | ||
| /// code | ||
| std::optional<std::reference_wrapper<const cluster::topic_metadata>> |
There was a problem hiding this comment.
@mmaslankaprv thanks for the comment.
It might be worthwhile to consider making this harder to use incorrectly. Perhaps by wrapping in a type that can't be copied or moved, so that it doesn't escape the scope as easily. The issue is that it really looks like it has value semantics at the call site:
auto metadata = _topics.local().get_topic_metadata_ref(tp_ns);
The other issue with visually seeing value semantics, is that the call site might not have a scheduling point to worry about, but might do something to the container that rebalances it--like any mutation.
There was a problem hiding this comment.
good point, i will consider wrapping it in a noncopyable nonmovable type.
There was a problem hiding this comment.
nbd. just thoughts in passing reading old PRs
Cover letter
Optimized per partition metadata lookup by storing partition metadata in
absl::btree_setthe set provides fast lookup while keeping partitionsordered. Limited number of times partition metadata are copied by
returning metadata object stored directly in a map.
Fixes: #4410
Fixes #4410
Release notes