-
Notifications
You must be signed in to change notification settings - Fork 141
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
Change emr job names based on the query type #2543
Change emr job names based on the query type #2543
Conversation
262fcb9
to
cc5321c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2543 +/- ##
=========================================
Coverage 95.41% 95.42%
Complexity 5027 5027
=========================================
Files 483 483
Lines 14016 14020 +4
Branches 944 944
=========================================
+ Hits 13374 13378 +4
Misses 621 621
Partials 21 21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
could we also add index name in batch query. Batch Query: clustername:flint_my_glue_default_http_logs_elb_and_requesturi_index:batch |
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.
Just wonder any max length limit in EMR job name?
cc5321c
to
23eaa3b
Compare
Yes addressing that even suresh raised this concern. 255 is the limit. |
Cluster name is between 3-28 chars, but index name can go beyond 255 characters. |
23eaa3b
to
94e1809
Compare
Signed-off-by: Vamsi Manohar <[email protected]>
94e1809
to
71333b3
Compare
jobName, | ||
clusterName, |
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.
What is the benefit of transferring the jobname formatting to the callee side? I'd rather leave them at handler level for consistency.
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 tried to do that but it requires a lot of refactoring and the current code structure is little convoluted.
Currently, sessionId creation happen inside CreateSessionRequest and so the jobName can't be built on the Callee side.
Signed-off-by: Vamsi Manohar <[email protected]> (cherry picked from commit 1a09f96) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit 1a09f96) Signed-off-by: Vamsi Manohar <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
* Batch Query:
clustername:batch
* Interactive Query:
clustername:interactive:sessionId
* Index Streaming Query:
clustername:streaming:flint_my_glue_default_http_logs_elb_and_requesturi_index
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.