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

Optionally compress OpenSky cache files #239

Merged
merged 7 commits into from
Sep 15, 2022

Conversation

aliaksei135
Copy link
Contributor

Adds option to compress cache files generated when the OpenSky Impala shell is queried for data.

  • Compresses all cache files for any method that calls the Impala shell if compress=True passed
  • When reading, checks if file is gzipped and transparently falls back to plain text file object if not
  • Tests modified to test both compressed and uncompressed scenarios

Fixes #237

@xoolive
Copy link
Owner

xoolive commented Sep 15, 2022

Thank you @aliaksei135, could you please check the fixes I implemented, in particular:

  • I added a context manager to properly handle open files (those were never closed);
  • I removed the try/except for two reasons:
    • you assumed a BadGzip exception meant you have a text file (it can also be a corrupted gzip file);
    • for now, the compress option will be False by default, so the exception would have been raised almost every time (which is not so great if you consider exceptions as exceptional)

If it's all good with you, I can merge that.

@xoolive
Copy link
Owner

xoolive commented Sep 15, 2022

Also, the tests were not passing in the gzip branch because cache files in text format remained there :)

@aliaksei135
Copy link
Contributor Author

Looks like late night coding is not for me... ;)

Thanks for catching those. Fixes LGTM.

Regarding the context manager not closing files, I thought this initially as well however it seems that they are closed in this kind of construct. Here is a test snippet:

def getcache():
    return open('test', 'w')

with getcache() as f:
    f.write('123')
    print(f.closed)
print(f.closed)

False
True

Happy to go with current fixes however

@xoolive xoolive merged commit 61f779e into xoolive:master Sep 15, 2022
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.

Compression of OpenSky cache
2 participants