-
Notifications
You must be signed in to change notification settings - Fork 59
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
vdk-impala: add Out Of Memory error handling #2747
vdk-impala: add Out Of Memory error handling #2747
Conversation
Add Out Of Memory error handling logic in the vdk-impala error handler.
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.
Looks good to me! My comments are mostly "clean code" type of comments.
projects/vdk-plugins/vdk-impala/src/vdk/plugin/impala/impala_error_handler.py
Outdated
Show resolved
Hide resolved
projects/vdk-plugins/vdk-impala/src/vdk/plugin/impala/impala_error_handler.py
Show resolved
Hide resolved
projects/vdk-plugins/vdk-impala/src/vdk/plugin/impala/impala_error_handler.py
Outdated
Show resolved
Hide resolved
projects/vdk-plugins/vdk-impala/src/vdk/plugin/impala/impala_error_handler.py
Outdated
Show resolved
Hide resolved
projects/vdk-plugins/vdk-impala/src/vdk/plugin/impala/impala_error_handler.py
Outdated
Show resolved
Hide resolved
review feedback
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 am wondering if we want to write a functional test that simulates out of memory query. That might not be easy.
Regardless you should test it against real system and describe how you test it in Testing Done in the PR.
(hint: you probably know but you can install it in your existing prod or test vdk env with pip install -e <vdk-impal-dir>
)
projects/vdk-plugins/vdk-impala/src/vdk/plugin/impala/impala_error_handler.py
Outdated
Show resolved
Hide resolved
projects/vdk-plugins/vdk-impala/src/vdk/plugin/impala/impala_error_handler.py
Outdated
Show resolved
Hide resolved
projects/vdk-plugins/vdk-impala/src/vdk/plugin/impala/impala_error_handler.py
Outdated
Show resolved
Hide resolved
I tested these changes on our testing impala instance together with our Infra team and the logic for this change is the result of our testing and research we did around these issues and the Impala memory configuration and settings. |
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.
Thanks. Looks good to me.
projects/vdk-plugins/vdk-impala/src/vdk/plugin/impala/impala_memory_error_handler.py
Outdated
Show resolved
Hide resolved
That was a bit unspecific. When something is tested manually, it's really good idea to describe it in a way that make the test reproducible by someone else. This way it can be peer-reviewed as well better. For example. What is the configuration of the testing Impala instance used, what types of queries were executed, what specific memory and performance metrics were tracked and used for verification. In general you can summarize the research that led to the logic implemented for handling memory issues. |
projects/vdk-plugins/vdk-impala/src/vdk/plugin/impala/impala_memory_error_handler.py
Outdated
Show resolved
Hide resolved
Review feedback
review feedback
Add Out Of Memory error handling logic in the vdk-impala error handler.