new fields in rpcInvokeReqExtra #1508
Conversation
runtime-light/tl/tl-types.h
Outdated
| // ===== RPC ===== | ||
|
|
||
| namespace exactlyOnce { | ||
| class Uuid final { |
There was a problem hiding this comment.
Please, check current approach for type naming in the tl namespace
| bool fetch(tl::fetcher& tlf) noexcept { | ||
| bool ok{fields_mask.fetch(tlf)}; | ||
|
|
||
| if (ok) { |
There was a problem hiding this comment.
Why not just ok = ok && trace_id.fetch()?
There was a problem hiding this comment.
That's not the case we had discussion about, but I it's to keep it as it is
|
|
||
| array out{std::pair{underline, mixed{}}, | ||
| std::pair{string{persistent_query_uuid_sv.data(), static_cast<string::size_type>(persistent_query_uuid_sv.size())}, mixed{}}, | ||
| std::pair{string{persistent_slot_uuid_sv.data(), static_cast<string::size_type>(persistent_slot_uuid_sv.size())}, mixed{}}}; |
There was a problem hiding this comment.
Why do we have an empty persistent_slot_uuid in the prepareRequest case? I think it would be more correct for it to be absent
| constexpr std::string_view lo_sv = "lo"; | ||
| constexpr std::string_view hi_sv = "hi"; | ||
|
|
||
| const auto underline = string{"_", 1}; |
There was a problem hiding this comment.
NIT: include please string header
runtime/interface.cpp
Outdated
|
|
||
| array out{std::pair{underline, mixed{}}, | ||
| std::pair{string{persistent_query_uuid_sv.data(), static_cast<string::size_type>(persistent_query_uuid_sv.size())}, mixed{}}, | ||
| std::pair{string{persistent_slot_uuid_sv.data(), static_cast<string::size_type>(persistent_slot_uuid_sv.size())}, mixed{}}}; |
There was a problem hiding this comment.
Same as for runtime-light
| constexpr std::string_view lo_sv = "lo"; | ||
| constexpr std::string_view hi_sv = "hi"; | ||
|
|
||
| const auto underline = string{"_", 1}; |
|
|
||
| const auto underline = string{"_", 1}; | ||
|
|
||
| array out{std::pair{underline, mixed{}}, |
There was a problem hiding this comment.
We may eliminate one array lookup here. To do that, create and set the array directly in if branches
| [&superglobals](const auto& value) noexcept { | ||
| using value_t = std::remove_cvref_t<decltype(value)>; | ||
|
|
||
| constexpr std::string_view exactlyOnce_prepareRequest_sv = "exactlyOnce.prepareRequest"; |
There was a problem hiding this comment.
nit: no actual need for _sv suffix
| extra.opt_persistent_query->request); | ||
| } | ||
| if (extra.opt_trace_context) { | ||
| const auto& trace_context{*extra.opt_trace_context}; |
There was a problem hiding this comment.
nit: let's not mix -> and * operators of std::optional. Just choose one
| constexpr std::string_view debug_flag_sv = "debug_flag"; | ||
|
|
||
| array out{ | ||
| std::pair{string{"_", 1}, mixed{string{tracing_traceContext_sv.data(), static_cast<string::size_type>(tracing_traceContext_sv.size())}}}, |
There was a problem hiding this comment.
nit: here you don't create a constant for underscore, but you do that above, why?
btw it's called underscore, not underline
| out.set_value(string{source_id_sv.data(), static_cast<string::size_type>(source_id_sv.size())}, | ||
| string{opt_source_id_value.data(), static_cast<string::size_type>(opt_source_id_value.size())}); | ||
| } | ||
| if (trace_context.reserved_status_0) { |
There was a problem hiding this comment.
I think this won't work when you try to use out array in further RPC calls. It should not contain these flags. It only needs to contain fields mask
added:
persistent_query,trace_context,execution_context