datalake/coordinator: introduce table_commit_builder #24855
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrwng
reviewed
Jan 17, 2025
CI test resultstest results on build#60913
test results on build#60960
|
dotnwat
reviewed
Jan 18, 2025
|
It’s not about forwarding reference but rather the output iterator concept.
The forwarding reference could be noise that I added unintentionally.
_As I refactored the code from chucked_vector& to output iterator version
it complained that lvalue is expected but std::back_inserter is giving me a
temporary so I changed that to Out&& from Out&._
…On Sat, 18 Jan 2025 at 00:43, Noah Watkins ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/v/datalake/coordinator/iceberg_file_committer.cc
<#24855 (comment)>
:
> @@ -194,6 +197,37 @@ checked<iceberg::partition_key, file_committer::errc> build_partition_key(
return iceberg::partition_key{
std::make_unique<iceberg::struct_value>(std::move(pk_res.value()))};
}
+
+template<typename Out>
+requires std::output_iterator<Out, iceberg::data_file>
+checked<std::nullopt_t, file_committer::errc> collect_icb_files(
+ const model::topic& topic,
+ const iceberg::table_metadata& table,
+ const iceberg::manifest_io& io,
+ const chunked_vector<data_file>& files,
+ Out&& icb_files) {
chunked_vector<data_file>* makes sense to me. an l-value reference would
also be ok i think, but i don't see how the forwarding reference is
communicating intent.
—
Reply to this email directly, view it on GitHub
<#24855 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEETWNTUB7FPI2GMAKHVAL2LGPSZAVCNFSM6AAAAABVMITHAKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKNRQGIYTANZVGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Simpler code. Simpler to reuse from other functions.
Want to reuse it for DLQ files.
commit_topic_files_to_catalog is too unwieldy. Simplify it before adding DLQ logic.
nvartolomei
force-pushed
the
nv/datalake-file-committer-refactor
branch
from
3849b89 to
df5c182
Compare
Refactor the iceberg file committer so that it is _easier_ to extend to manage multiple tables (i.e. main table and the DLQ table).
nvartolomei
force-pushed
the
nv/datalake-file-committer-refactor
branch
2 times, most recently
from
db6d3fe to
3f0e068
Compare
nvartolomei
changed the title
Retry command for Build#60960please wait until all jobs are finished before running the slash command |
andrwng
approved these changes
Jan 21, 2025
|
/ci-repeat 1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor the iceberg file committer so that it is easier to extend to manage multiple tables (i.e. main table and the DLQ table).
Backports Required
Release Notes