Skip to content

Improve performance of generic repo #155

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elprans
Copy link
Member

@elprans elprans commented Apr 8, 2025

The APT and RPM repos work on a locally-synced copy of their S3 prefixes
(because the repo tooling expects repos to be in a local fs), but the
generic repo currently just reads/writes things to S3 directly. This is
problematic for atomicity and performance reasons.

Fix this by doing the S3 sync for the generic repo as well. While at
it, lower log levels on some things to reduce noise.

This was tested locally (running process_incoming.py on artifacts
produced with local gel-pkg runs).

@elprans elprans requested a review from Copilot April 10, 2025 20:34
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

server/process_incoming.py:749

  • Verify that the 'tf.extract' function supports the 'filter' parameter; otherwise, this change could lead to extraction issues or compatibility problems.
tf.extract(member, staging_dir, filter="data")

server/process_incoming.py:383

  • It may be safer to verify that the metadata file exists before attempting to open it, to avoid potential runtime errors if the expected file is missing.
with open(f"{metadata_file}.metadata.json") as f:

bucket.objects.filter(Prefix=obj_key).delete()
for filename in versions[ver]:
logger.info("Deleting outdated: %s", filename)
filename.unlink()
Copy link
Preview

Copilot AI Apr 10, 2025

Choose a reason for hiding this comment

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

Consider adding exception handling around 'filename.unlink()' to ensure that a failure during file deletion does not crash the application unexpectedly.

Suggested change
filename.unlink()
try:
filename.unlink()
except FileNotFoundError:
logger.warning("File not found during deletion: %s", filename)
except PermissionError:
logger.error("Permission denied during deletion: %s", filename)
except Exception as e:
logger.error("Unexpected error during deletion of %s: %s", filename, e)

Copilot uses AI. Check for mistakes.

@elprans elprans force-pushed the work-on-generic-locally branch from d8b6406 to 34415fb Compare April 10, 2025 21:15
The APT and RPM repos work on a locally-synced copy of their S3 prefixes
(because the repo tooling expects repos to be in a local fs), but the
generic repo currently just reads/writes things to S3 directly.  This is
problematic for atomicity and performance reasons.

Fix this by doing the S3 sync for the generic repo as well.  While at
it, lower log levels on some things to reduce noise.
@elprans elprans force-pushed the work-on-generic-locally branch from 34415fb to 37540dd Compare April 10, 2025 22:47
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.

1 participant