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

feat: update database with validators report #383

Merged
merged 9 commits into from
Apr 23, 2024
Merged

feat: update database with validators report #383

merged 9 commits into from
Apr 23, 2024

Conversation

cka-y
Copy link
Contributor

@cka-y cka-y commented Apr 10, 2024

Summary:
This PR introduces a new cloud function tasked with the role of updating the database with entities derived from the GTFS validation report.

Expected Behavior:
Upon completion of the JSON reports' upload to the mobilitydata-feeds-[dev | qa | prod] bucket by the gtfs web validator, this function is triggered via the gtfs_validator_execution workflow. This ensures that the database is promptly updated with the latest GTFS validation results.

Testing Tips:

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with ./scripts/api-tests.sh to make sure you didn't break anything
  • Add or update any needed documentation to the repo
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

@cka-y cka-y linked an issue Apr 10, 2024 that may be closed by this pull request
@cka-y cka-y marked this pull request as draft April 11, 2024 03:35
@Alessandro100
Copy link
Contributor

Great PR! It was easy to follow what's going on with all the comments and code structure

@davidgamez davidgamez mentioned this pull request Apr 19, 2024
5 tasks
@cka-y cka-y changed the title feat: update Database with validators report feat: update database with validators report Apr 22, 2024
@cka-y cka-y marked this pull request as ready for review April 22, 2024 16:53
Comment on lines 137 to 138
logging.info(f"Validation report {report_id} already exists. Terminating.")
raise Exception(f"Validation report {report_id} already exists.")
Copy link
Member

Choose a reason for hiding this comment

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

Assuming that cloud functions can be called multiple times with the same parameters, It's safe to log a warning and not raise an error. Raising an error could cause infinite loops, or at least multiple calls to the cloud function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did change the log from info to warning, but the thrown exception is caught line 204:

# Generate the database entities required for the report
  try:
      entities = generate_report_entities(
          version,
          validated_at,
          json_report,
          dataset_stable_id,
          session,
          feed_stable_id,
      )
  except Exception as error:
      return str(error), 409  # Conflict if report already exists

Do you think it still requires refactoring of some sort?

Copy link
Member

Choose a reason for hiding this comment

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

The question is, if the GCP gets 409, will it re-send the event?
My suggestion is if the function detects that the report is already created, don't send an error status back as it's expected message gets duplicated in the cloud environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding the retry happens when an exception is thrown and not caught (https://cloud.google.com/functions/docs/samples/functions-tips-retry). I tested and the function do not seem to retry when the same request is done twice (duplicate id) -- here are the logs

@cka-y cka-y requested a review from davidgamez April 22, 2024 18:14
@cka-y cka-y merged commit 675e33d into main Apr 23, 2024
2 checks passed
@cka-y cka-y deleted the feat/229 branch April 23, 2024 19:18
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.

Update Database with validators report
3 participants