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

Fix Session state bug and improve Query Efficiency in REPL #245

Merged
merged 3 commits into from
Feb 9, 2024

Conversation

kaituo
Copy link
Collaborator

@kaituo kaituo commented Feb 6, 2024

Description

This PR introduces a bug fix and enhancements to FlintREPL's session management and optimizes query execution methods. It addresses a specific issue where marking a session as 'dead' inadvertently triggered the creation of a new, unnecessary session. This behavior resulted in the new session entering a spin-wait state, leading to duplicate jobs.

The improvements include:

  • Introduction of earlyExitFlag: A new flag, earlyExitFlag, has been introduced and is set to true under two conditions: when a job is excluded or when it is not associated with the current session's job run ID. This flag is evaluated in the shutdown hook to determine whether the session state should be marked as 'dead'. This change effectively prevents the unintended creation of duplicate sessions by the SQL plugin, ensuring resources are utilized more efficiently.
  • Query Method Optimization: The method for executing queries has been shifted from scrolling to search, eliminating the need for creating unnecessary scroll contexts. This adjustment enhances the performance and efficiency of query operations.
  • Reversion of Previous Commit: The PR reverts a previous change (be82024) following the resolution of the related issue in the SQL plugin ([BUG] Cannot invoke \"java.util.Collection.size()\" because \"this.exprValues\" is null  sql#2436), further streamlining the operation and maintenance of the system.

Testing:

  1. Integration tests were added to cover both REPL and streaming job functionalities, ensuring the robustness of the fixes.
  2. Manual testing was conducted to validate the bug fix.

Issues Resolved

#246

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.

@penghuo
Copy link
Collaborator

penghuo commented Feb 7, 2024

Query Method Optimization: The method for executing queries has been shifted from scrolling to search, eliminating the need for creating unnecessary scroll contexts. This adjustment enhances the performance and efficiency of query operations.

Thanks, just create an issue describe same issue. #246

@penghuo
Copy link
Collaborator

penghuo commented Feb 7, 2024

This change effectively prevents the unintended creation of duplicate sessions by the SQL plugin,
could u elaborate more on this? in which case SQL plugin will create duplicate session?

@kaituo
Copy link
Collaborator Author

kaituo commented Feb 7, 2024

This change effectively prevents the unintended creation of duplicate sessions by the SQL plugin,
could u elaborate more on this? in which case SQL plugin will create duplicate session?

FlintREPL has a shutdown hook to set the session state to dead. In our SQL setup, marking a session as 'dead' automatically triggers the creation of a new session. However, the newly created session (initiated by the control plane) will enter a spin-wait state, where it inefficiently waits until timeout, leading to unproductive resource consumption and eventual timeout. This PR added the earlyExitFlag flag and set it to true when the job is excluded or the job is not the current session job run id. Then in the shutdown hook, we check the earlyExitFlag before marking the session state as 'dead'. When this flag is true, it indicates that the control plane has already initiated a new session to handle remaining requests for the current session. Thereby we prevent unnecessary duplicate job creation.

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.

I made single line change on FlintJob in #241 but was unable to add IT. Could you merge and verify if it works as expected? Thanks!

Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

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

Thanks for the change!

This PR introduces a bug fix and enhancements to FlintREPL's session management and optimizes query execution methods. It addresses a specific issue where marking a session as 'dead' inadvertently triggered the creation of a new, unnecessary session. This behavior resulted in the new session entering a spin-wait state, leading to duplicate jobs.

The improvements include:
- **Introduction of `earlyExitFlag`**: A new flag, `earlyExitFlag`, has been introduced and is set to `true` under two conditions: when a job is excluded or when it is not associated with the current session's job run ID. This flag is evaluated in the shutdown hook to determine whether the session state should be marked as 'dead'. This change effectively prevents the unintended creation of duplicate sessions by the SQL plugin, ensuring resources are utilized more efficiently.
- **Query Method Optimization**: The method for executing queries has been shifted from scrolling to search, eliminating the need for creating unnecessary scroll contexts. This adjustment enhances the performance and efficiency of query operations.
- **Reversion of Previous Commit**: The PR reverts a previous change (opensearch-project@be82024) following the resolution of the related issue in the SQL plugin (opensearch-project/sql#2436), further streamlining the operation and maintenance of the system.

**Testing**:
1. Integration tests were added to cover both REPL and streaming job functionalities, ensuring the robustness of the fixes.
2. Manual testing was conducted to validate the bug fix.

Signed-off-by: Kaituo Li <[email protected]>
Signed-off-by: Kaituo Li <[email protected]>
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.

Thanks for the changes!

@kaituo kaituo merged commit 9c15194 into opensearch-project:main Feb 9, 2024
4 checks passed
@opensearch-trigger-bot
Copy link

The backport to 0.1 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/opensearch-spark/backport-0.1 0.1
# Navigate to the new working tree
pushd ../.worktrees/opensearch-spark/backport-0.1
# Create a new branch
git switch --create backport/backport-245-to-0.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9c151946937e56bbee76e0ed8ca53cb763b21aad
# Push it to GitHub
git push --set-upstream origin backport/backport-245-to-0.1
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/opensearch-spark/backport-0.1

Then, create a pull request where the base branch is 0.1 and the compare/head branch is backport/backport-245-to-0.1.

kaituo added a commit to kaituo/opensearch-spark that referenced this pull request Feb 9, 2024
…h-project#245)

* Fix Session state bug and improve Query Efficiency in REPL

This PR introduces a bug fix and enhancements to FlintREPL's session management and optimizes query execution methods. It addresses a specific issue where marking a session as 'dead' inadvertently triggered the creation of a new, unnecessary session. This behavior resulted in the new session entering a spin-wait state, leading to duplicate jobs.

The improvements include:
- **Introduction of `earlyExitFlag`**: A new flag, `earlyExitFlag`, has been introduced and is set to `true` under two conditions: when a job is excluded or when it is not associated with the current session's job run ID. This flag is evaluated in the shutdown hook to determine whether the session state should be marked as 'dead'. This change effectively prevents the unintended creation of duplicate sessions by the SQL plugin, ensuring resources are utilized more efficiently.
- **Query Method Optimization**: The method for executing queries has been shifted from scrolling to search, eliminating the need for creating unnecessary scroll contexts. This adjustment enhances the performance and efficiency of query operations.
- **Reversion of Previous Commit**: The PR reverts a previous change (opensearch-project@be82024) following the resolution of the related issue in the SQL plugin (opensearch-project/sql#2436), further streamlining the operation and maintenance of the system.

**Testing**:
1. Integration tests were added to cover both REPL and streaming job functionalities, ensuring the robustness of the fixes.
2. Manual testing was conducted to validate the bug fix.

Signed-off-by: Kaituo Li <[email protected]>

* added Java doc

Signed-off-by: Kaituo Li <[email protected]>

* fix IT by restore env variable change

Signed-off-by: Kaituo Li <[email protected]>

---------

Signed-off-by: Kaituo Li <[email protected]>
kaituo added a commit that referenced this pull request Feb 12, 2024
* Fix Session state bug and improve Query Efficiency in REPL

This PR introduces a bug fix and enhancements to FlintREPL's session management and optimizes query execution methods. It addresses a specific issue where marking a session as 'dead' inadvertently triggered the creation of a new, unnecessary session. This behavior resulted in the new session entering a spin-wait state, leading to duplicate jobs.

The improvements include:
- **Introduction of `earlyExitFlag`**: A new flag, `earlyExitFlag`, has been introduced and is set to `true` under two conditions: when a job is excluded or when it is not associated with the current session's job run ID. This flag is evaluated in the shutdown hook to determine whether the session state should be marked as 'dead'. This change effectively prevents the unintended creation of duplicate sessions by the SQL plugin, ensuring resources are utilized more efficiently.
- **Query Method Optimization**: The method for executing queries has been shifted from scrolling to search, eliminating the need for creating unnecessary scroll contexts. This adjustment enhances the performance and efficiency of query operations.
- **Reversion of Previous Commit**: The PR reverts a previous change (be82024) following the resolution of the related issue in the SQL plugin (opensearch-project/sql#2436), further streamlining the operation and maintenance of the system.

**Testing**:
1. Integration tests were added to cover both REPL and streaming job functionalities, ensuring the robustness of the fixes.
2. Manual testing was conducted to validate the bug fix.



* added Java doc



* fix IT by restore env variable change



---------

Signed-off-by: Kaituo Li <[email protected]>
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.

3 participants