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.

Sign up for GitHub

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

feat(core-clp): Postpone GlobalMetadataDB updates until archive writing finishes (fixes #685). #705

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

feat(core-clp): Postpone GlobalMetadataDB updates until archive writing finishes (fixes #685). #705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
1c13138
first draft
davemarco Jan 30, 2025
faecdf9
fix lint
davemarco Jan 30, 2025
5d7aa3b
remove unnesary changes
davemarco Jan 30, 2025
91f868f
change to add
davemarco Jan 31, 2025
719b97b
save work
davemarco Jan 31, 2025
4f1e6c6
new method
davemarco Feb 4, 2025
962d61f
revert hpp
davemarco Feb 4, 2025
957edbf
revert more old changes
davemarco Feb 4, 2025
3b0b37a
latest
davemarco Feb 4, 2025
8e59e74
fix lint
davemarco Feb 4, 2025
8aaff6f
latest
davemarco Feb 4, 2025
363cd56
fix lint
davemarco Feb 4, 2025
48b0779
haiqi review
davemarco Feb 6, 2025
846062b
haiqi review
davemarco Feb 6, 2025
920cfb8
fix lint
davemarco Feb 6, 2025
0628636
haiqi review
davemarco Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions components/core/src/clp/streaming_archive/writer/Archive.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,6 @@ void Archive::open(UserConfig const& user_config) {

m_global_metadata_db = user_config.global_metadata_db;

m_global_metadata_db->open();
m_global_metadata_db->add_archive(m_id_as_string, *m_local_metadata);
m_global_metadata_db->close();

m_file = nullptr;

// Open log-type dictionary
Expand Down Expand Up @@ -238,8 +234,14 @@ void Archive::close() {

m_metadata_file_writer.close();

update_global_metadata();
m_global_metadata_db = nullptr;

for (auto* file : m_file_metadata_for_global_update) {
delete file;
}
m_file_metadata_for_global_update.clear();

m_metadata_db.close();

m_creator_id_as_string.clear();
Expand Down Expand Up @@ -557,8 +559,6 @@ void Archive::persist_file_metadata(vector<File*> const& files) {

m_metadata_db.update_files(files);

m_global_metadata_db->update_metadata_for_files(m_id_as_string, files);

// Mark files' metadata as clean
for (auto file : files) {
file->mark_metadata_as_clean();
Expand Down Expand Up @@ -593,16 +593,15 @@ void Archive::close_segment_and_persist_file_metadata(

for (auto file : files) {
file->mark_as_in_committed_segment();
// Files in this collection only hold metadata. Emplaced files have called method
// `File::append_to_segment()` which deallocates memory for timestamp,
// logtype, and variable fields.
m_file_metadata_for_global_update.emplace_back(file);
davemarco marked this conversation as resolved.
Show resolved Hide resolved
}

m_global_metadata_db->open();
persist_file_metadata(files);
update_metadata();
m_global_metadata_db->close();

for (auto file : files) {
delete file;
}
files.clear();
}

Expand Down Expand Up @@ -635,8 +634,6 @@ void Archive::update_metadata() {
m_metadata_file_writer.seek_from_begin(0);
m_local_metadata->write_to_file(m_metadata_file_writer);

m_global_metadata_db->update_archive_metadata(m_id_as_string, *m_local_metadata);

if (m_print_archive_stats_progress) {
nlohmann::json json_msg;
json_msg["id"] = m_id_as_string;
Expand All @@ -647,6 +644,19 @@ void Archive::update_metadata() {
}
}

auto Archive::update_global_metadata() -> void {
m_global_metadata_db->open();
if (false == m_local_metadata.has_value()) {
throw OperationFailed(ErrorCode_Failure, __FILENAME__, __LINE__);
}
m_global_metadata_db->add_archive(m_id_as_string, m_local_metadata.value());
m_global_metadata_db->update_metadata_for_files(
m_id_as_string,
m_file_metadata_for_global_update
);
m_global_metadata_db->close();
}
Comment on lines +647 to +658
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling and condition check.

The implementation is correct, but there are a few improvements that can be made:

  1. Use idiomatic condition check
  2. Add descriptive error message

Apply this diff to improve the code:

 auto Archive::update_global_metadata() -> void {
     m_global_metadata_db->open();
-    if (false == m_local_metadata.has_value()) {
-        throw OperationFailed(ErrorCode_Failure, __FILENAME__, __LINE__);
+    if (!m_local_metadata) {
+        throw OperationFailed(
+            ErrorCode_Failure,
+            __FILENAME__,
+            __LINE__,
+            "Local metadata not initialized before updating global metadata"
+        );
     }
     m_global_metadata_db->add_archive(m_id_as_string, m_local_metadata.value());
     m_global_metadata_db->update_metadata_for_files(
             m_id_as_string,
             m_file_metadata_for_global_update
     );
     m_global_metadata_db->close();
 }

Committable suggestion skipped: line range outside the PR's diff.


// Explicitly declare template specializations so that we can define the template methods in this
// file
template void Archive::write_log_event_ir<eight_byte_encoded_variable_t>(
Expand Down
7 changes: 7 additions & 0 deletions components/core/src/clp/streaming_archive/writer/Archive.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,11 @@ class Archive {
*/
void update_metadata();

/**
* Updates metadata in the global metadata database.
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto comment for deleting m_files_written.

You might also need to mention about exceptions if m_global_metadata_db's api throws any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the delete out of the function. None of the functions I am calling have any documented exceptions in the .hpp file.

*/
auto update_global_metadata() -> void;

// Variables
boost::uuids::uuid m_id;
std::string m_id_as_string;
Expand Down Expand Up @@ -316,6 +321,8 @@ class Archive {
std::vector<File*> m_files_with_timestamps_in_segment;
std::vector<File*> m_files_without_timestamps_in_segment;

std::vector<File*> m_file_metadata_for_global_update;

size_t m_target_segment_uncompressed_size;
Segment m_segment_for_files_with_timestamps;
ArrayBackedPosIntSet<logtype_dictionary_id_t>
Expand Down
Loading