Async Executor Service Depedencies Refactor #2488
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2488 +/- ##
============================================
+ Coverage 95.41% 95.43% +0.01%
- Complexity 5006 5024 +18
============================================
Files 481 483 +2
Lines 13963 13989 +26
Branches 940 943 +3
============================================
+ Hits 13323 13350 +27
+ Misses 618 617 -1
Partials 22 22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
vamsi-amazon
force-pushed
the
main
branch
11 times, most recently
from
36b84fd
to
6cee349
Compare
vamsi-amazon
requested review from
ps48,
kavithacm,
derek-ho,
joshuali925,
dai-chen,
YANG-DB,
rupal-bq,
mengweieric,
Swiddis,
penghuo,
seankao-az,
MaxKsyunz,
Yury-Fridlyand,
anirudha,
forestmvey and
acarbonetto
as code owners
vamsi-amazon
force-pushed
the
main
branch
2 times, most recently
from
339b727
to
e5a1837
Compare
vamsi-amazon
force-pushed
the
main
branch
3 times, most recently
from
813b424
to
3d17b63
Compare
legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlStatsAction.java
Show resolved
Hide resolved
spark/src/main/java/org/opensearch/sql/spark/client/EMRServerlessClientFactory.java
Show resolved
Hide resolved
.../src/test/java/org/opensearch/sql/spark/transport/config/AsyncExecutorServiceModuleTest.java
Show resolved
Hide resolved
spark/src/main/java/org/opensearch/sql/spark/transport/config/AsyncExecutorServiceModule.java
Show resolved
Hide resolved
| validateSparkExecutionEngineConfig(sparkExecutionEngineConfig); | ||
| if (isNewClientCreationRequired(sparkExecutionEngineConfig.getRegion())) { | ||
| region = sparkExecutionEngineConfig.getRegion(); | ||
| this.emrServerlessClient = createEMRServerlessClient(this.region); |
There was a problem hiding this comment.
If client changed, how to get job status of in previous region?
do we need to shutdown underlying EMRClient?
There was a problem hiding this comment.
usecase I am targeting is customer initially didn't configure spark execution engine cluster setting(or configured wrong region) and once the customer configures we create a new client. Also, Lazy client creation when async api is first invoked.
If customer changes region in between, then it will result in errors for old jobs. Customer needs to be careful enough.[Do you see this as issue?]
There was a problem hiding this comment.
understood, could we add notes in our doc to alert customer?
There was a problem hiding this comment.
Addressing in this PR: #2493
Signed-off-by: Vamsi Manohar <reddyvam@amazon.com>
Description
Restructured the instantiation of AsyncQueryExecutorService using the Guice framework.
Substituted the usage of EMRServerlessClient with EMRServerlessClientFactory. This modification helps prevent unnecessary errors and warnings during the plugin bootstrapping phase, particularly when constructing EMRServerlessClient using SparkExecutionEngineConfig.
Previously, updating the Spark execution engine cluster configuration necessitated a system restart. With the current implementation, the recreation of EMRServerlessClient upon any cluster setting update eliminates the need for a restart.
statement count and session count metrics are blocking operations as they make calls to opensearch index. So, we are moving stats operations to thread pool instead of processing on Transport thread.
Issues Resolved
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.