Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Introduce Source::setMinimumTileUpdateInterval API #16416

Merged
merged 9 commits into from
Apr 20, 2020

Conversation

pozdnyakov
Copy link
Contributor

The Source::setMinimumTileUpdateInterval(Duration) method sets the minimum tile update interval, which is used to throttle the tile update network requests.

The corresponding Source::getMinimumTileUpdateInterval() getter is added too.

Default minimum tile update interval value is Duration::zero().

Tag https://github.com/mapbox/mapbox-gl-native-team/issues/335

@pozdnyakov pozdnyakov self-assigned this Apr 20, 2020
@pozdnyakov pozdnyakov force-pushed the mikhail_source_min_tile_update_interval branch from fc67f52 to ad7313d Compare April 20, 2020 13:02
@pozdnyakov pozdnyakov added the needs changelog Indicates PR needs a changelog entry prior to merging. label Apr 20, 2020
@pozdnyakov pozdnyakov marked this pull request as ready for review April 20, 2020 13:48
Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

lgtm

@alexshalamov
Copy link
Contributor

/cc @asheemmamoowala do you think that property would be useful for gl-js as well?

@asheemmamoowala
Copy link
Contributor

@asheemmamoowala do you think that property would be useful for gl-js as well?

@pozdnyakov Is there any documentation around what this method is supposed to do? Tracking for parity at mapbox/mapbox-gl-js#9603, but I don't see any use case for it in GL-JS as yet.

Introduce `Resource::minimumUpdateInterval` and consider it in the online file source.

The `minimumUpdateInterval` is used to throttle the requests, which were initiated due to resource expiration.
The `Source::setMinimumTileUpdateInterval()` method sets the minimum tile update interval, which is used to throttle the tile update network requests.

Default value is `Duration::zero()`.
@pozdnyakov pozdnyakov force-pushed the mikhail_source_min_tile_update_interval branch from ad7313d to 0c4cbaa Compare April 20, 2020 16:11
@pozdnyakov pozdnyakov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Apr 20, 2020
@pozdnyakov
Copy link
Contributor Author

@pozdnyakov Is there any documentation around what this method is supposed to do?

Just in this pr description, and the inline comments in source.hpp.. However, it's quite simple: it makes sure the update tile network request does not happen more often than the given duration (even if the server-provided expiration date supposes more frequent tile updates)

@pozdnyakov pozdnyakov merged commit f513dc9 into master Apr 20, 2020
@pozdnyakov pozdnyakov deleted the mikhail_source_min_tile_update_interval branch April 20, 2020 18:54
std::unique_ptr<AsyncRequest> req = fs->request(resource, [&](Response) {
auto wait = util::now() - start;
EXPECT_GE(wait, resource.minimumUpdateInterval);
EXPECT_LT(wait, resource.minimumUpdateInterval + Milliseconds(10));
Copy link
Contributor

Choose a reason for hiding this comment

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

1: [ RUN ] OnlineFileSource.RespectMinimumUpdateInterval
1: ../test/storage/online_file_source.test.cpp:232: Failure
1: Expected: (wait) < (resource.minimumUpdateInterval + Milliseconds(10)), actual: 8-byte object <02-00 00-00 00-00 00-00> vs 8-byte object <80-60 33-3C 00-00 00-00>

This one here is flaking.

@@ -76,6 +79,14 @@ class Source : public mbgl::util::noncopyable {
void setPrefetchZoomDelta(optional<uint8_t> delta) noexcept;
optional<uint8_t> getPrefetchZoomDelta() const noexcept;

// If the given source supports loading tiles from a server,
// sets the minimum tile update interval, which is used to
// throttle the tile update network requests.
Copy link
Contributor

@chloekraw chloekraw Apr 22, 2020

Choose a reason for hiding this comment

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

nit: "suppress" might be a better word here, if we're preventing the update network requests from happening entirely. "throttle" sounds like the request is still made but we're slowing them down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In our requests are still made but less frequently, is "suppress" still a better word?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, so my thinking is, update network requests that are more frequent than the minimumTileUpdateInterval are suppressed (i.e., not made at all) -- is that accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, we can put it that way, I'll update this comment in #16422

@chloekraw
Copy link
Contributor

@pozdnyakov a few follow-up questions:

  • can Source::setMinimumTileUpdateInterval(Duration) be safely called at any time?
  • once a TTL override is set, can the same API call be used to change the override? For example, if a device were on low power mode, you might want to slow down a source's refresh rate even further.

cc @mapbox/maps-android @knov to cut tickets for platform bindings. For now, it's only a high priority on Android.

cc @asheemmamoowala

@pozdnyakov
Copy link
Contributor Author

  • can Source::setMinimumTileUpdateInterval(Duration) be safely called at any time?

yeah

  • once a TTL override is set, can the same API call be used to change the override? For example, if a device were on low power mode, you might want to slow down a source's refresh rate even further.

absolutely, the resulting network request interval is always = max(TTL, minimuTileUpdateInterval /* set by the introduced API*/)

pozdnyakov added a commit that referenced this pull request Apr 22, 2020
pozdnyakov added a commit that referenced this pull request Apr 22, 2020
pozdnyakov added a commit that referenced this pull request Apr 22, 2020
pozdnyakov added a commit that referenced this pull request Apr 22, 2020
@chloekraw
Copy link
Contributor

thanks for the clarification @pozdnyakov!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants