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.

Sign up for GitHub

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

Jump to bottom

Optimize prepareStatementExecution request freq #828

Merged
merged 2 commits into from
Oct 30, 2024
Merged

Optimize prepareStatementExecution request freq #828

merged 2 commits into from
Oct 30, 2024

Conversation

noCharger
Copy link
Collaborator

@noCharger noCharger commented Oct 29, 2024 •
edited
Loading

Description

Optimize prepareStatementExecution request freq.

Before the change:

  • prepareStatementExecution will run k times, depends dynamically on session duration and query execution time.
  • getNext will run k times to create new reader clients

After the change:

  • prepareStatementExecution will run 1 time before the query loop
  • getNext will behave the same, to avoid concurrent issue of using one reader client within same session

Also tested on EMR to verify number of times.

Related Issues

#804

Check List

  • Implemented unit tests
  • Commits are signed per the DCO using --signoff

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.

@noCharger noCharger marked this pull request as ready for review October 30, 2024 16:45
dai-chen
dai-chen reviewed Oct 30, 2024
View reviewed changes
Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

Is it possible to reproduce the issue in UT or IT and verify the fix?

spark-sql-application/src/main/scala/org/apache/spark/sql/StatementExecutionManagerImpl.scala Show resolved Hide resolved
@noCharger
Copy link
Collaborator Author

Is it possible to reproduce the issue in UT or IT and verify the fix?

There's UT in https://github.com/opensearch-project/opensearch-spark/pull/828/files#diff-19a51bd4a67b635f750a1f694cf276cd220c549fa48f80e1018782b64d32ec61, are you expect something different?

@dai-chen
Copy link
Collaborator

Is it possible to reproduce the issue in UT or IT and verify the fix?

There's UT in https://github.com/opensearch-project/opensearch-spark/pull/828/files#diff-19a51bd4a67b635f750a1f694cf276cd220c549fa48f80e1018782b64d32ec61, are you expect something different?

I mean is there UT to cover this case? The existing UT passed even if there was infinity loop bug right?

@noCharger
Copy link
Collaborator Author

Is it possible to reproduce the issue in UT or IT and verify the fix?

There's UT in https://github.com/opensearch-project/opensearch-spark/pull/828/files#diff-19a51bd4a67b635f750a1f694cf276cd220c549fa48f80e1018782b64d32ec61, are you expect something different?

I mean is there UT to cover this case? The existing UT passed even if there was infinity loop bug right?

The existing UT expect prepareStatementExecution running k times, and we changed it to be one here

@noCharger noCharger merged commit a2a9838 into opensearch-project:main Oct 30, 2024
4 checks passed
@noCharger noCharger self-assigned this Oct 30, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 30, 2024
@github-actions
Optimize prepareStatementExecution request freq (#828)
0fe4509 
* Optimize prepareStatementExecution request freq

Signed-off-by: Louis Chu <[email protected]>

* Add UT

Signed-off-by: Louis Chu <[email protected]>

---------

Signed-off-by: Louis Chu <[email protected]>
(cherry picked from commit a2a9838)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Oct 30, 2024
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 30, 2024
@github-actions
Optimize prepareStatementExecution request freq (#828)
e623ed9 
* Optimize prepareStatementExecution request freq

Signed-off-by: Louis Chu <[email protected]>

* Add UT

Signed-off-by: Louis Chu <[email protected]>

---------

Signed-off-by: Louis Chu <[email protected]>
(cherry picked from commit a2a9838)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Oct 30, 2024
Merged
noCharger pushed a commit that referenced this pull request Oct 30, 2024
@opensearch-trigger-bot @github-actions
Optimize prepareStatementExecution request freq (#828) (#841)
64e8a30 
* Optimize prepareStatementExecution request freq



* Add UT



---------


(cherry picked from commit a2a9838)

Signed-off-by: Louis Chu <[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>
seankao-az pushed a commit that referenced this pull request Oct 30, 2024
@opensearch-trigger-bot @github-actions
Optimize prepareStatementExecution request freq (#828) (#842)
f52317f 
* Optimize prepareStatementExecution request freq



* Add UT



---------


(cherry picked from commit a2a9838)

Signed-off-by: Louis Chu <[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>
kenrickyap pushed a commit to Bit-Quill/opensearch-spark that referenced this pull request Dec 11, 2024
@noCharger @14yapkc1
Optimize prepareStatementExecution request freq (opensearch-project#828)
d2f975b 
* Optimize prepareStatementExecution request freq

Signed-off-by: Louis Chu <[email protected]>

* Add UT

Signed-off-by: Louis Chu <[email protected]>

---------

Signed-off-by: Louis Chu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dai-chen dai-chen dai-chen approved these changes

@rupal-bq rupal-bq Awaiting requested review from rupal-bq

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@vmmusings vmmusings Awaiting requested review from vmmusings

@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@kaituo kaituo Awaiting requested review from kaituo kaituo is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@LantaoJin LantaoJin Awaiting requested review from LantaoJin LantaoJin is a code owner

@ykmr1224 ykmr1224 Awaiting requested review from ykmr1224 ykmr1224 is a code owner

Assignees

@noCharger noCharger

Labels
0.5.1 backport 0.5-nexus backport 0.5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@noCharger @dai-chen