Skip to content

Comments

feat: try to use the S3 tiles version if available instead of returning 503#126

Open
juanmahidalgo wants to merge 3 commits intomasterfrom
feat/try-to-use-s3
Open

feat: try to use the S3 tiles version if available instead of returning 503#126
juanmahidalgo wants to merge 3 commits intomasterfrom
feat/try-to-use-s3

Conversation

@juanmahidalgo
Copy link
Contributor

@juanmahidalgo juanmahidalgo commented Feb 17, 2025

Atlas Server - S3 Redirect Enhancement

Context

Currently, when the map is not ready, the server returns a 503 status code. However, we have a feature flag that allows redirecting to S3 where we store cached versions of the tiles. This PR enhances the logic to use S3 redirects when available, even when the map is not ready.
In addition, the PR includes:

  • Replace programEntryPoint deprecreated function in favor of run.
  • Add a retries mechanism to the start method of the map component in case some of it's logic throws an error. It will try 10 times to start the service.
    E.g of an error seen that didn't let the component to start:
    [ERROR] (Map component): Failed to initialize map component: Error: Error fetching rentals, the server responded with: 502 "Failed to initialize map component: Error: Error fetching rentals, the server responded with: 502"

Changes

  • Modified the tiles handlers (createTilesRequestHandler and createLegacyTilesRequestHandler) to:
    1. Check for S3 redirect feature flag first
    2. If enabled and S3 URL is available, redirect to S3 (regardless of map ready state)
    3. If enabled but no S3 URL available, fall back to map state check
    4. If disabled, maintain original behavior

Logic Flow

When map is not ready:

  • S3 feature OFF → 503 "Not ready"
  • S3 feature ON + S3 URL available → 301 redirect to S3
  • S3 feature ON + S3 URL not available → 503 "Not ready" + warning log

When map is ready:

  • S3 feature OFF → serve from map
  • S3 feature ON + S3 URL available → 301 redirect to S3
  • S3 feature ON + S3 URL not available → serve from map + warning log

Testing

Added comprehensive test suite covering all possible combinations of:

  • Map ready state (ready/not ready)
  • S3 redirect feature flag (ON/OFF)
  • S3 URL availability (available/not available)

Benefits

  • Improved availability: Can serve tiles even when map is not ready if we have a cached version in S3
  • Better resilience: Clear fallback behavior when S3 is not available
  • Maintainable code: Common logic extracted into shared functions
  • Full test coverage: All edge cases are tested

@coveralls
Copy link

coveralls commented Feb 17, 2025

Pull Request Test Coverage Report for Build 13368654260

Details

  • 103 of 204 (50.49%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+5.4%) to 45.528%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/controllers/routes.ts 0 1 0.0%
src/index.ts 0 1 0.0%
src/service.ts 0 9 0.0%
src/modules/map/component.ts 0 90 0.0%
Files with Coverage Reduction New Missed Lines %
src/service.ts 1 0.0%
Totals Coverage Status
Change from base Build 13284995313: 5.4%
Covered Lines: 1733
Relevant Lines: 3963

💛 - Coveralls

Copy link
Contributor

@LautaroPetaccio LautaroPetaccio left a comment

Choose a reason for hiding this comment

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

Looks great 👍

* Checks if the S3 redirect feature flag is enabled
*/
const isS3RedirectEnabled = async (features: IFeaturesComponent) => {
return await features.getIsFeatureEnabled(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return await features.getIsFeatureEnabled(
return features.getIsFeatureEnabled(

const result = await handler({ url: new URL('http://localhost') }) as HandlerResponse
expect(result.status).toBe(200)
expect(loggerWarnMock).toHaveBeenCalledWith('No S3 file available')
expect(result.headers?.['content-type']).toBe('application/json')
Copy link
Contributor

Choose a reason for hiding this comment

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

This line, which is used in all tests that assert that the tiles are returned is not making sure the tiles are being returned as expected, as it only checks that the response is of type application/json. Would you mind changing the assertion to do so?

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.

3 participants