-
Notifications
You must be signed in to change notification settings - Fork 223
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
bug-1921849: support elasticsearch 8 #6741
Conversation
d96b425
to
9ac223f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
32912f9
to
b360970
Compare
This comment was marked as resolved.
This comment was marked as resolved.
19b3ea4
to
c0e2cfd
Compare
Co-authored-by: krzepka <[email protected]>
c0e2cfd
to
2704c10
Compare
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to do this PR in two passes. This is a code read pass.
While you're fixing things I brought up, I'll spend some time going through some manual testing for things I'm wondering about.
Then after you make changes, I'll read through those and add anything that came up in manual testing.
b460f21
to
b3a3c73
Compare
except elasticsearch.exceptions.TransportError as e: | ||
# If this is a TransportError, we try to figure out what the error | ||
except elasticsearch.BadRequestError as e: | ||
# If this is a BadRequestError, we try to figure out what the error | ||
# is and fix the document and try again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this section removes fields that cause document_parsing_exception
and retries the document. This seems like an odd choice given that it happens after value fixing occurs, which should already be preventing the three types of failure we catch here. The only way i can think of to reach this code block in production is if we are writing to a field not in our mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to be the case that it wrote all the data into Elasticsearch even if it wasn't in the mapping. That way when they add new fields to the crash report, they'd get indexed even if Socorro didn't explicitly have support for it. While the intentions were good, that was terrible so I changed it such that it only indexes what's defined in super search fields and in the mapping.
9988801
to
0e51573
Compare
0e51573
to
0b51672
Compare
0b51672
to
8cedc0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through and tested the issues I raised in the previous review.
bin/process_crashes.sh
works now.- Things correctly wait for both es and legacy_es containers to start up.
- TopCrashers works now.
I wrote up bug 1933824 about a curiosity I hit when uploading some crash report dump files to gcs emulator. That's not related to these changes.
My only issue is that I'm not sure why you added four new metrics to statsd_metrics.yml
rather than just the one I was hitting issues with.
Everything else looks fine as far as I can tell.
r+wc
use
ELASTICSEARCH_MODE=PREFER_NEW
to make the webapp use es8 and the processor write to both es 1.4 and es8