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

Jump to bottom

Logging of schema migrations #7070

Merged
merged 4 commits into from
Oct 26, 2023
Merged

Logging of schema migrations #7070

merged 4 commits into from
Oct 26, 2023

Conversation

jedelbo
Copy link
Contributor

@jedelbo jedelbo commented Oct 19, 2023

What, How & Why?

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed.

@jedelbo jedelbo requested a review from tgoyne October 19, 2023 13:37
@coveralls-official
Copy link

coveralls-official bot commented Oct 20, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build github_pull_request_281342

  • 129 of 153 (84.31%) changed or added relevant lines in 7 files are covered.
  • 860 unchanged lines in 36 files lost coverage.
  • Overall coverage increased (+0.03%) to 90.562%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/realm/db.cpp 24 29 82.76%
src/realm/object-store/object_store.cpp 65 70 92.86%
src/realm/replication.cpp 21 35 60.0%
Files with Coverage Reduction New Missed Lines %
src/realm/object-store/impl/list_notifier.cpp 1 98.67%
src/realm/sync/noinst/client_impl_base.hpp 1 93.93%
test/test_dictionary.cpp 1 99.85%
src/realm/sync/instruction_applier.cpp 2 64.42%
src/realm/sync/noinst/server/server_history.cpp 2 67.69%
src/realm/util/compression.cpp 2 91.34%
src/realm/uuid.cpp 2 97.06%
src/realm/util/file.cpp 3 81.25%
src/realm/util/serializer.cpp 3 90.03%
test/test_index_string.cpp 3 94.01%
Totals Coverage Status
Change from base Build github_pull_request_281218: 0.03%
Covered Lines: 233253
Relevant Lines: 257563
💛 - Coveralls

@jedelbo jedelbo requested review from nicola-cab and removed request for tgoyne October 23, 2023 07:14
nicola-cab
nicola-cab reviewed Oct 25, 2023
View reviewed changes
Copy link
Member

@nicola-cab nicola-cab left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I am not sure whether measuring the time that the migration function has taken is ok to have or not.

src/realm/object-store/object_store.cpp
uint64_t target_schema_version, SchemaMode mode,
std::vector<SchemaChange> const& changes, bool handle_automatically_backlinks,
std::function<void()> migration_function)
{
create_metadata_tables(group);
using namespace std::chrono;
auto t1 = steady_clock::now();
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you want to measure this every time we do a migration? From the code, it seems we expect the logger to be always on. It shouldn't be a huge pessimization, but isn't the migration a hot path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You will only do migration once. And getting the time does not cost.

@nicola-cab nicola-cab self-requested a review October 26, 2023 12:27
@jedelbo jedelbo merged commit 2dca8e7 into next-major Oct 26, 2023
1 of 2 checks passed
@jedelbo jedelbo deleted the je/log-migration branch October 26, 2023 12:45
@jedelbo jedelbo mentioned this pull request Oct 30, 2023
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nicola-cab nicola-cab nicola-cab approved these changes

Assignees

@jedelbo jedelbo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jedelbo @nicola-cab