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

Add /refresh to update sources and some configurations #1179

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

Conversation

mesudBe-Orbit
Copy link

@mesudBe-Orbit mesudBe-Orbit commented Feb 6, 2024

Try to fix #288

  • Inject Arc<RwLock> to server, T: state.tiles, state.sprites, state.fonts, catalog, config
  • Add a /refresh to endpoint(read and merge config and flush the cache)
  • Add test to tesh.sh
  • Update doc

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

looks like a good start, thanks! I will have to think of how to test this best - perhaps add a new shell script test-watch.sh that is similar to existing test.sh, except that it will:

  • start martin with --auto-bounds skip (so that it starts faster)
  • wait for it to start
  • get a catalog into catalog1.json
  • use psql to create a new table and a function, and possibly make some other modifications like deleting something else
  • use curl to POST a refresh command to some Martin's endpoint that triggers the refresh - I guess this request should not return until refresh is done? Make sure to have some global "refresh lock" to avoid multiple simultaneous refresh requests?
  • get another catalog saving it to catalog2.txt

The existing bless/diff setup will check that the catalog results are as expected

martin/src/srv/server.rs Outdated Show resolved Hide resolved
@mesudBe-Orbit mesudBe-Orbit mentioned this pull request Feb 7, 2024
Co-authored-by: Yuri Astrakhan <yuriastrakhan@gmail.com>
@sharkAndshark
Copy link
Collaborator

sharkAndshark commented May 6, 2024

Some ways to push this forward?

@sharkAndshark
Copy link
Collaborator

I'm trying to keep going on this.

@nyurik
Copy link
Member

nyurik commented May 27, 2024

There is a well known dashmap crate that essentially functions as a concurrent hashmap (i.e. replaces RwLock<HashMap<K, V>>). We may want to use that as a simpler alternative to store sources / fonts / sprites. Or it might be a bad idea - only time will tell.

@sharkAndshark
Copy link
Collaborator

And should we keep the watch config option like if the watch is enabled the /refresh would be added and if it's disabled would be not.

@nyurik
Copy link
Member

nyurik commented May 31, 2024

lets implement /refresh separately - there are a lot of functionality required to make this work - so i think first we should get all the plumbing in place to make it possible, and then we will introduce:

  • watching file system for changes (e.g. pmtiles / mbtiles / sprints / fonts)
  • timer based refresh of postgres
  • calling POST on /refresh, possibly restricting it to localhost only

@sharkAndshark sharkAndshark force-pushed the feature/dynamic-config-reload branch from 94365d7 to 7440428 Compare June 3, 2024 16:26
@sharkAndshark
Copy link
Collaborator

Seems it's almost there? Will fix the tests and update doc later.

@sharkAndshark
Copy link
Collaborator

sharkAndshark commented Jun 5, 2024

Weired. That shouldn't fail. CI / Build and test docker images

Testing points3857_srid_0_0_0.pbf from http://localhost:3111/points3857/0/0/0
***** Test server response for PMTiles source *****
Testing pmt.json from http://localhost:3111/stamen_toner__raster_CC-BY-ODbL_z3
curl: (22) The requested URL returned error: 404

The source stamen_toner__raster_CC-BY-ODbL_z3 has been configured actually.

[2024-06-05T02:55:58Z WARN  martin::utils::id_resolver] Source `stamen_toner__raster_CC-BY+ODbL_z3` (/tests/fixtures/pmtiles/stamen_toner__raster_CC-BY+ODbL_z3.pmtiles) was renamed to `stamen_toner__raster_CC-BY-ODbL_z3`. Source IDs must be unique, cannot be reserved, and must contain alpha-numeric characters or `._-`
[2024-06-05T02:55:58Z INFO  martin::file_config] Configured source stamen_toner__raster_CC-BY-ODbL_z3 from /tests/fixtures/pmtiles/stamen_toner__raster_CC-BY+ODbL_z3.pmtiles

@sharkAndshark sharkAndshark marked this pull request as ready for review June 6, 2024 07:18
@sharkAndshark
Copy link
Collaborator

sharkAndshark commented Jun 6, 2024

There must be a lot to clean up.

@sharkAndshark sharkAndshark changed the title Changing all endpoint functions - /catalog Add /refresh to update sources and some configurations Jun 7, 2024
@sharkAndshark sharkAndshark requested a review from nyurik June 7, 2024 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WATCH_MODE info
3 participants