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

Catch KeyError from collector from missing TLE #116

Merged
merged 7 commits into from
Sep 6, 2022

Conversation

TAlonglong
Copy link
Collaborator

Now and then I get for some reason an exception as described in #30

What happens is then that the subscriber for this area stops and thus can not continue collecting new segments.
Other regions continue to work if there are more. Or the the geographic gatherer just continue to run.

One way to handle this, as proposed in this PR, is to catch the KeyError exception and then continue keeping the subscriber running. Therefore at the next run it will try again and it will still work.

I think this is better than just stop working. At least it tries again.

Another way is to do this is to make the whole geographic gatherer stop and then leave to the user how he or she will handle this. But this will affect all the regions in the gatherer.

@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #116 (3deb327) into main (84b4dc0) will increase coverage by 0.25%.
The diff coverage is 100.00%.

❗ Current head 3deb327 differs from pull request most recent head b280d7f. Consider uploading reports for the commit b280d7f to get more accurate results

@@            Coverage Diff             @@
##             main     #116      +/-   ##
==========================================
+ Coverage   89.58%   89.84%   +0.25%     
==========================================
  Files          25       25              
  Lines        3611     3653      +42     
==========================================
+ Hits         3235     3282      +47     
+ Misses        376      371       -5     
Impacted Files Coverage Δ
pytroll_collectors/tests/test_region_collector.py 96.96% <100.00%> (+0.22%) ⬆️
pytroll_collectors/tests/test_triggers.py 100.00% <100.00%> (ø)
pytroll_collectors/triggers/_base.py 83.84% <100.00%> (+4.31%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@TAlonglong TAlonglong self-assigned this Sep 2, 2022
@TAlonglong TAlonglong requested review from mraspaud and pnuu and removed request for mraspaud September 2, 2022 05:54
Copy link
Member

@pnuu pnuu left a comment

Choose a reason for hiding this comment

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

In general LTGM, just a question on the test of raised KeyError.

'uid': 'AVHRR_C_EUMP_20220901102203_51653_eps_o_amv_l2d.bin',
'origin': '157.249.16.188:9062',
'end_time': datetime(2022, 9, 1, 10, 25, 3)})
assert "Found no TLE entry for 'METOP-B' to similate KeyError" in caplog.text
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert "Found no TLE entry for 'METOP-B' to similate KeyError" in caplog.text
assert "Found no TLE entry for 'METOP-B' to simulate" in caplog.text

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, where does this exception text originate? I found "Found no TLE entry for '%s'" in https://github.com/pytroll/pyorbital/blob/main/pyorbital/tlefile.py#L200 but the rest of it I can't seem to find in either Pyorbital, Pytroll Schedule nor Pytroll Collectors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, how to explain

I just added some dummy text in the test to be sure I matched the test KeyError message and not the message from the actual code. If that make any sense. I struggled a bit before I got this right.

But sure, I can at least get the spelling right.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right. Didn't grep the same file 😅

@pnuu pnuu merged commit 81ee9b2 into pytroll:main Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants