datalake: manage protobuf descriptor lifetimes correctly #24154
Conversation
| @@ -96,7 +96,7 @@ struct schema_translating_visitor { | |||
| auto type = type_to_iceberg(*d).value(); | |||
| co_return type_and_buf{ | |||
| .type = resolved_type{ | |||
| .schema = *d, | |||
| .schema = wrapped_protobuf_descriptor { *d, pb_def }, | |||
There was a problem hiding this comment.
It's possible, pb_def is just a wrapper around a shared_ptr to the underlying descriptor pool though so it won't change much. I'll switch to moving it though in case the shared_ptr is ever switched to something else.
| @@ -96,7 +96,7 @@ struct schema_translating_visitor { | |||
| auto type = type_to_iceberg(*d).value(); | |||
| co_return type_and_buf{ | |||
| .type = resolved_type{ | |||
| .schema = *d, | |||
There was a problem hiding this comment.
Can we also document this in the descriptor method? Thanks!
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/58172#01933d6a-d016-478f-bf8e-39fb39696db4 |
ballard26
force-pushed
the
iceberg-microbench
branch
from
8d658a3 to
ca9b97f
Compare
|
the below tests from https://buildkite.com/redpanda/redpanda/builds/58178#01933ddf-68cc-4fd5-98f3-80b61e9d5d8e have failed and will be retried |
|
non flaky failures in https://buildkite.com/redpanda/redpanda/builds/58178#01933e3b-6273-4d21-9d8c-6a0366fcf3d3: |
Retry command for Build#58178please wait until all jobs are finished before running the slash command |
|
/backport v24.3.x |
| @@ -96,7 +96,7 @@ struct schema_translating_visitor { | |||
| auto type = type_to_iceberg(*d).value(); | |||
| co_return type_and_buf{ | |||
| .type = resolved_type{ | |||
| .schema = *d, | |||
| .schema = wrapped_protobuf_descriptor { *d, std::move(pb_def) }, | |||
There was a problem hiding this comment.
//
descriptorreferences an object owned byschema_def.
but what makes the guarantee that movingpb_defdoesn't invalidate that same ownership property that it looks like you are trying to maintain?
There was a problem hiding this comment.
Good call, right now its indirection. I.e, the descriptor pool is held by pb_def in a shared_ptr. It may be better to move pb_def first then get the reference though in case that changes in the future.
There was a problem hiding this comment.
ahh i see its heap allocated. got it thanks
The protobuf descriptor reference passed around by
resolved_schemais to an object owned by a descriptor poolin
pandaproxy::schema_registry::protobuf_schema_definition. Therefore this PR changesresolved_schemato also pass aroundprotobuf_schema_definitionso that the lifetime of the reference doesn't exceed that of the object.A follow-up PR to this one will include unit tests that will fail without this change.
Backports Required
Release Notes