Test FLX sync geospatial queries #7042

ironage · 2023-10-06T23:47:11Z

Fixes #7007
Updates the BAAS version we test with to today's version.

danieltabacaru · 2023-10-07T06:31:17Z

Some tests fail because the server sends a different error message unrelated to geospatial. You can either fix them, or use an older baas version and I'll fix the tests. I can provide the baas version you can use.

danieltabacaru · 2023-10-11T12:46:19Z

Can we update baas to an even newer version and bump go version to 1.21 (required by latest baas)? Actually, the upgrade happened on October 5th (65d3d091584f47fe8c84d184ae753f7015f59e95), so how is baas still working?

coveralls-official · 2023-10-12T19:32:06Z

Pull Request Test Coverage Report for Build james.stone_419

129 of 129 (100.0%) changed or added relevant lines in 4 files are covered.
72 unchanged lines in 14 files lost coverage.
Overall coverage increased ( 0.004%) to 91.662%

Files with Coverage Reduction	New Missed Lines	%
src/realm/sync/network/http.cpp	2	63.29%
src/realm/table_view.cpp	2	94.18%
src/realm/uuid.cpp	2	97.06%
src/realm/sync/network/network.cpp	3	89.86%
src/realm/sync/noinst/server/server_history.cpp	3	67.85%
src/realm/table.cpp	3	90.63%
src/realm/util/serializer.cpp	3	90.03%
src/realm/sync/noinst/client_reset_operation.cpp	4	87.23%
src/realm/sync/noinst/protocol_codec.hpp	4	76.05%
src/realm/alloc_slab.cpp	5	92.89%

Totals
Change from base Build 1815:	0.004%
Covered Lines:	230803
Relevant Lines:	251799

💛 - Coveralls

danieltabacaru · 2023-10-19T12:26:23Z

src/realm/sync/noinst/client_reset.cpp

@@ -655,7  652,7 @@ LocalVersionIDs perform_client_reset_diff(DBRef db_local, DBRef db_remote, sync:
        // partially recovered, but interrupted may continue sync the next time it is
        // opened with only partially recovered state while having lost the history of any
        // offline modifications.
-        history_local->set_client_file_ident_in_wt(wt_local->get_version(), client_file_ident);
+        history_local->set_client_file_ident_in_wt(wt_local->get_version(), remote_ident);


Doesn't the server complain that we BIND needing a file ident which the server provides, and then we IDENT with a different file ident? I was wondering if we need to restart the session 🤔

One curiosity, why can't we set the file ident at line 632? (would've helped if the comment at line 631 explained why)

I think we don't set the ident on 632 because if the recovery is interrupted at some point before finishing, we still want the server to send a client reset due to having the old ident. In FLX recovery there is a series of commits and we only want to use a new ident once everything has been successfully committed.

The server doesn't seem to complain about using the other IDENT. But that change isn't necessary here, and I think I'll restore the original just in case it does in fact matter to the server somehow.

CHANGELOG.md

ironage · 2023-10-20T19:34:46Z

src/realm/sync/subscriptions.cpp

@@ -352,6  352,18 @@ void MutableSubscriptionSet::insert_sub(const Subscription& sub)
    m_subs.push_back(sub);
 }

+void MutableSubscriptionSet::on_boostrap_cleared()
+{
+    update_state(State::Pending);


How is setting the subscription state back to pending helping?

Fair point, setting back to the pending state isn't actually necessary. I can revert this. The main thing is that we want this subscription set to be picked up to send to the server again. This is done by get_next_pending_version() which queries for "(pending OR boostrapping) and snapshot_version > client_upload_version" so the only relevant change is to update the snapshot version at this point so that we restart the bootstrap process of this subscription with the server.

src/realm/sync/subscriptions.cpp

CHANGELOG.md

src/realm/sync/subscriptions.cpp

danieltabacaru · 2023-11-01T06:44:46Z

@ironage My fixes are in, so you should be able to continue the work on this.

download/upload/advance may have already happened by the time the test code gets to the wait_for_upload() line which results in the test hanging forever because there are no further changes. A fix is to do a timed wait for the expected state which will succeed immediately if the same race occurs.

ironage · 2023-11-01T17:29:59Z

evergreen/install_baas.sh

-    --configFile=etc/configs/test_config.json --configFile="${BASE_PATH}/config_overrides.json" > "${BAAS_SERVER_LOG}" 2>&1 &
+    --configFile=etc/configs/test_config.json --configFile=etc/configs/test_rcore_config.json > "${BAAS_SERVER_LOG}" 2>&1 &


Tyler requested that the BAAS team maintain the config overrides for our tests so that they can keep them up to date with server features. I've removed our local overrides file evergreen/config_overrides.json and now this points to a config in their repo https://github.com/10gen/baas/blob/master/etc/configs/test_rcore_config.json

Nice! I will eventually move our dependency definitions (e.g. STITCH_SUPPORT_LIB_URL) to use the baas definitions as well, but they need to add the definitions for Ubuntu before this happens.

danieltabacaru · 2023-11-01T21:05:49Z

test/object-store/sync/flx_migration.cpp

@@ -441,8  441,12 @@ TEST_CASE("Test client migration and rollback with recovery", "[sync][flx][flx m
    //  Migrate back to FLX - and keep the realm session open
    trigger_server_migration(session.app_session(), MigrateToFLX, logger_ptr);

-    REQUIRE(!wait_for_upload(*outer_realm));
-    REQUIRE(!wait_for_download(*outer_realm));
+    // wait for the subscription store to initialize after downloading


Instead of polling, one other way to fix this (I think) is to commit a change and keep the completion callbacks.

test/object-store/sync/flx_sync.cpp

danieltabacaru · 2023-11-01T21:24:58Z

test/object-store/sync/flx_sync.cpp

@@ -2047,17  2094,18 @@ TEST_CASE("flx: interrupted bootstrap restarts/recovers on reconnect", "[sync][f
    }

    auto realm = Realm::get_shared_realm(interrupted_realm_config);
-    auto table = realm->read_group().get_table("class_TopLevel");


I think this should work:

auto table = realm->read_group().get_table("class_TopLevel"); realm->get_latest_subscription_set().get_state_change_notification(sync::SubscriptionSet::State::Complete).get(); realm->refresh(); REQUIRE(table->size() == obj_ids_at_end.size()); for (auto& id : obj_ids_at_end) { REQUIRE(table->find_primary_key(Mixed{id})); }

Same below.

My guess as to why I saw a failure here was that the server marks the subscription here as complete even with no objects, because the server hasn't processed the object insertions server-side so the translator doesn't know about them yet. Isn't that possible?

hmm..I thought the issue is that the realm has the data but it's pinned to an older version. I think we wait until the server integrates the changes, but maybe they are not in mongo at the point bootstrap starts and a snapshot is needed (so we could poll on that)

That does sound possible, but it's something the test can check for (by querying the mongo collection and waiting for it to be populated). I think the current changes here are making the test incorrectly accept some invalid behaviors (e.g. it would pass if an interrupted bootstrap always resulted in the subscription set immediately being marked as complete). I think all of the introductions of timed_wait_for() here are incorrect and are just making the tests pass even when something went wrong.

Fair points. I will add some waits on the mongo server collection state and go back to relying on the subscription notifications and see what happens.

If polling on the mongo collection doesn't work, another idea would be to poll on a different Realm than the one being tested, so that you can make strict expectations about how the realm under test should behave given the observed state.

danieltabacaru

Nice work!

michael-wb

The changes look good - are there going to be additional changes to the tests per the comments?

ironage · 2023-11-02T22:56:01Z

are there going to be additional changes to the tests per the comments?

No, I've removed the test polling code but I think polling the server backend is unnecessary after Daniel's fixes which resolved a few test hangs. We will just have to wait and see if there are any further test hangs and address them as the come up.
Just waiting on final confirmation from the server team that 8db0179 is not going to cause unwanted side effects.

ironage added the no-changelog label Oct 6, 2023

ironage requested a review from michael-wb October 6, 2023 23:47

ironage self-assigned this Oct 6, 2023

ironage marked this pull request as draft October 16, 2023 17:46

danieltabacaru reviewed Oct 19, 2023

View reviewed changes

ironage force-pushed the js/test-geo-flx branch 3 times, most recently from 35238cb to 95f10f2 Compare October 20, 2023 18:18

ironage commented Oct 20, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

kiburtse mentioned this pull request Oct 20, 2023

Random CI failures due to timeouts #7071

Closed

ironage commented Oct 20, 2023

View reviewed changes

src/realm/sync/subscriptions.cpp Outdated Show resolved Hide resolved

danieltabacaru reviewed Oct 21, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

danieltabacaru reviewed Oct 23, 2023

View reviewed changes

src/realm/sync/subscriptions.cpp Outdated Show resolved Hide resolved

This was referenced Oct 23, 2023

FLX subscriptions are not sent to the server #7076

Closed

FLX subscriptions are not re-sent to the server if bootstrap is interrupted #7077

Closed

ironage added 3 commits November 1, 2023 10:14

add tests for FLX geospatial

f8d94db

update tests for new BAAS error messages

f361c5c

ironage force-pushed the js/test-geo-flx branch from 95f10f2 to 3caa441 Compare November 1, 2023 17:26

ironage commented Nov 1, 2023

View reviewed changes

ironage marked this pull request as ready for review November 1, 2023 20:28

ironage requested a review from danieltabacaru November 1, 2023 20:51

danieltabacaru reviewed Nov 1, 2023

View reviewed changes

test/object-store/sync/flx_sync.cpp Outdated Show resolved Hide resolved

danieltabacaru reviewed Nov 1, 2023

View reviewed changes

danieltabacaru approved these changes Nov 1, 2023

View reviewed changes

revert polling waits in tests

12ccd1c

michael-wb approved these changes Nov 2, 2023

View reviewed changes

kiburtse mentioned this pull request Nov 2, 2023

Sync client assertion failure in unbinded session: Assertion failed: m_initiated #7074

Closed

fix an error if recovering schema changes in dev mode

8db0179

danieltabacaru mentioned this pull request Nov 2, 2023

Bump Go version used to build BAAS #7107

Closed

ironage and others added 5 commits November 3, 2023 11:17

add additional tests

72a3b80

Merge branch 'master' into js/test-geo-flx

104ae79

don't overcomplicate the test

86d63dd

lint

2a4fdcc

Merge branch 'master' into js/test-geo-flx

95ebaaa

danieltabacaru merged commit 7556b53 into master Nov 7, 2023
33 of 34 checks passed

danieltabacaru deleted the js/test-geo-flx branch November 7, 2023 10:50

rorbech mentioned this pull request Nov 7, 2023

Write self-maintained build script for building BaaS locally realm/realm-kotlin#1562

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test FLX sync geospatial queries #7042

Test FLX sync geospatial queries #7042

ironage commented Oct 6, 2023

danieltabacaru commented Oct 7, 2023

danieltabacaru commented Oct 11, 2023 •

edited

Loading

coveralls-official bot commented Oct 12, 2023 •

edited

Loading

danieltabacaru Oct 19, 2023 •

edited

Loading

danieltabacaru Oct 19, 2023

ironage Oct 19, 2023

ironage Oct 19, 2023

ironage Oct 20, 2023

danieltabacaru commented Nov 1, 2023

ironage Nov 1, 2023

michael-wb Nov 1, 2023

danieltabacaru Nov 1, 2023

danieltabacaru Nov 1, 2023

ironage Nov 1, 2023

danieltabacaru Nov 1, 2023

tgoyne Nov 1, 2023 •

edited

Loading

ironage Nov 1, 2023

tgoyne Nov 1, 2023

danieltabacaru left a comment

michael-wb left a comment

ironage commented Nov 2, 2023 •

edited

Loading

		--configFile=etc/configs/test_config.json --configFile="${BASE_PATH}/config_overrides.json" > "${BAAS_SERVER_LOG}" 2>&1 &
		--configFile=etc/configs/test_config.json --configFile=etc/configs/test_rcore_config.json > "${BAAS_SERVER_LOG}" 2>&1 &

Test FLX sync geospatial queries #7042

Test FLX sync geospatial queries #7042

Conversation

ironage commented Oct 6, 2023

danieltabacaru commented Oct 7, 2023

danieltabacaru commented Oct 11, 2023 • edited Loading

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

Pull Request Test Coverage Report for Build james.stone_419

💛 - Coveralls

danieltabacaru Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgoyne Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru left a comment

Choose a reason for hiding this comment

michael-wb left a comment

Choose a reason for hiding this comment

ironage commented Nov 2, 2023 • edited Loading

danieltabacaru commented Oct 11, 2023 •

edited

Loading

coveralls-official bot commented Oct 12, 2023 •

edited

Loading

danieltabacaru Oct 19, 2023 •

edited

Loading

tgoyne Nov 1, 2023 •

edited

Loading

ironage commented Nov 2, 2023 •

edited

Loading