Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump CAF to version 0.18.6 #2693

Merged
merged 1 commit into from Dec 16, 2022
Merged

Bump CAF to version 0.18.6 #2693

merged 1 commit into from Dec 16, 2022

Conversation

patszt
Copy link
Contributor

@patszt patszt commented Nov 9, 2022

This PR bumps the CAF version to 0.18.6.
Some of the major weird things happening in the review:

  • Many config parsing now have .data() added to string_view. This is added because the get_or calls require the last argument (which is now std::string_view) to have a type_id. The type_id requires the type to be serializable. I dind't come up with a good way to deserialize into std::string_view as it doesn't own a buffer. This is why for now i have just used .data(). In CAF 0.19 this is fixed i believe

@patszt patszt force-pushed the topic/caf-bump-to-18.6 branch 3 times, most recently from 38be554 to d67a75f Compare November 10, 2022 14:05
@patszt patszt force-pushed the topic/caf-bump-to-18.6 branch 9 times, most recently from afe39f8 to 684377c Compare December 1, 2022 12:39
@patszt patszt force-pushed the topic/caf-bump-to-18.6 branch 8 times, most recently from 4c44b05 to 0788a10 Compare December 8, 2022 12:55
@patszt patszt changed the title TEST PR testing CAF 0.18.6 cmake changes on CI Bump CAF to version 0.18.6 Dec 9, 2022
@patszt patszt marked this pull request as ready for review December 9, 2022 10:25
@patszt patszt added caf blocked Blocked by an (external) issue labels Dec 9, 2022
libvast/src/system/exporter.cpp Show resolved Hide resolved
libvast_test/vast/test/test.hpp Show resolved Hide resolved
libvast/test/table_slice.cpp Show resolved Hide resolved
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review round: Everything that relates to the build scaffolding and CI. I really like what I'm seeing so far!

.github/workflows/vast.yaml Outdated Show resolved Hide resolved
.github/workflows/vast.yaml Outdated Show resolved Hide resolved
libvast/CMakeLists.txt Outdated Show resolved Hide resolved
libvast/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/broker/CMakeLists.txt Show resolved Hide resolved
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second part of the review: lsvast.

tools/lsvast/src/print_partition.cpp Outdated Show resolved Hide resolved
tools/lsvast/src/print_partition.cpp Outdated Show resolved Hide resolved
tools/lsvast/src/print_partition.cpp Outdated Show resolved Hide resolved
tools/lsvast/src/print_partition.cpp Outdated Show resolved Hide resolved
tools/lsvast/src/print_partition.cpp Outdated Show resolved Hide resolved
tools/lsvast/src/print_segment.cpp Outdated Show resolved Hide resolved
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part three: All the builtins.

libvast/builtins/endpoints/export.cpp Outdated Show resolved Hide resolved
libvast/builtins/endpoints/status.cpp Show resolved Hide resolved
libvast/builtins/operators/drop.cpp Outdated Show resolved Hide resolved
libvast/builtins/operators/rename.cpp Outdated Show resolved Hide resolved
libvast/builtins/operators/rename.cpp Outdated Show resolved Hide resolved
libvast/builtins/stores/segment_store.cpp Show resolved Hide resolved
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part four: plugins.

plugins/parquet/parquet.cpp Outdated Show resolved Hide resolved
plugins/web/tests/authentication.cpp Outdated Show resolved Hide resolved
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part five: The test support library libvast_test.

libvast_test/src/events.cpp Outdated Show resolved Hide resolved
libvast_test/src/table_slices.cpp Outdated Show resolved Hide resolved
libvast_test/vast/test/test.hpp Outdated Show resolved Hide resolved
libvast_test/vast/test/test.hpp Outdated Show resolved Hide resolved
libvast_test/vast/test/type_ids.hpp Outdated Show resolved Hide resolved
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part six: FlatBuffers tables.

libvast/fbs/partition.fbs Show resolved Hide resolved
libvast/fbs/partition.fbs Outdated Show resolved Hide resolved
libvast/fbs/synopsis.fbs Outdated Show resolved Hide resolved
libvast/fbs/value_index.fbs Outdated Show resolved Hide resolved
libvast/fbs/value_index.fbs Outdated Show resolved Hide resolved
@dominiklohmann dominiklohmann removed the blocked Blocked by an (external) issue label Dec 10, 2022
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review round seven: About the first half of the libvast; I'm done for the day.

libvast/include/vast/bitvector.hpp Outdated Show resolved Hide resolved
libvast/include/vast/bloom_filter.hpp Outdated Show resolved Hide resolved
libvast/include/vast/chunk.hpp Outdated Show resolved Hide resolved
libvast/include/vast/chunk.hpp Outdated Show resolved Hide resolved
libvast/include/vast/concept/convertible/data.hpp Outdated Show resolved Hide resolved
libvast/include/vast/http_api.hpp Outdated Show resolved Hide resolved
libvast/include/vast/legacy_type.hpp Outdated Show resolved Hide resolved
libvast/include/vast/subnet.hpp Outdated Show resolved Hide resolved
libvast/include/vast/system/catalog.hpp Outdated Show resolved Hide resolved
libvast/include/vast/system/node_control.hpp Outdated Show resolved Hide resolved
@patszt patszt mentioned this pull request Dec 13, 2022
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round eight: The remainder of libvast.

libvast/include/vast/detail/legacy_deserialize.hpp Outdated Show resolved Hide resolved
libvast/include/vast/detail/settings.hpp Show resolved Hide resolved
libvast/src/format/json.cpp Outdated Show resolved Hide resolved
libvast/src/format/json.cpp Outdated Show resolved Hide resolved
libvast/src/format/test.cpp Outdated Show resolved Hide resolved
libvast/src/segment.cpp Outdated Show resolved Hide resolved
libvast/src/store.cpp Outdated Show resolved Hide resolved
libvast/src/system/default_configuration.cpp Show resolved Hide resolved
libvast/src/system/disk_monitor.cpp Show resolved Hide resolved
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly glanced at the tests as the changes were mostly mechanical. Initial testing shows that everything is working, even with an old database.

I would like to see this merged and then roll this out on our testbed for performance testing and to see what we need to tweak with the defaults.

There's one last warning I get when building, and this diff fixes it:

diff --git a/libvast/test/format/arrow.cpp b/libvast/test/format/arrow.cpp
index 54f3bacd9..a92fa5a0b 100644
--- a/libvast/test/format/arrow.cpp
+++ b/libvast/test/format/arrow.cpp
@@ -94,7 +94,8 @@ TEST(arrow IPC read) {
   format::arrow::writer writer;
   writer.out(stream);
   for (auto& slice : zeek_conn_log)
-    writer.write(slice);
+    if (auto err = writer.write(slice))
+      FAIL("failed to write zeek.conn log " << err);
   auto data = stream->Finish().ValueOrDie()->ToString();
   auto in = std::make_unique<std::istringstream>(std::string{data});
   auto options = caf::settings{};

This PR is a fantastic piece of work 🚀

libvast/test/system/configuration.cpp Outdated Show resolved Hide resolved
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed a bunch of things—only action left to do (assuming that CI runs through) is to squash and then merge!

libvast/CMakeLists.txt Outdated Show resolved Hide resolved
libvast/src/system/default_configuration.cpp Show resolved Hide resolved
@patszt patszt merged commit 4fcc772 into master Dec 16, 2022
@patszt patszt deleted the topic/caf-bump-to-18.6 branch December 16, 2022 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants