-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
KYLIN-4432 duplicated queries with sytax error take unexpect long time when lazy query enabled #1167
Conversation
…e when lazy query enabled
LGTM. How about extract evicting the dummy response logic to the finally block? |
@kyotoYaho Thanks for the suggestion. I updated the PR with a finally block. |
Codecov Report
@@ Coverage Diff @@
## master #1167 +/- ##
============================================
- Coverage 25.06% 25.01% -0.06%
- Complexity 6250 6253 +3
============================================
Files 1446 1449 +3
Lines 88309 88502 +193
Branches 12358 12374 +16
============================================
Hits 22136 22136
- Misses 64003 64189 +186
- Partials 2170 2177 +7
Continue to review full report at Codecov.
|
Hi xiacongling, the previous one without finally block is better. I'm sorry I mislead you. |
Never mind, @kyotoYaho ! I think the problem is that conditions are lost in finally block. We can either rebuild the conditions or use the cached value.
Using "try .. finally" is more elegant, but some code refactoring is needed to keep it clean, I think. To get out of the dilemma, we can treat this PR as a quick bug-fix, and discuss the refactoring in another issue. I will revert the commit. Thanks for the review! |
This reverts commit 8150221.
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.
+1 LGTM
…e when lazy query enabled (apache#1167) * KYLIN-4432 duplicated queries with sytax error take unexpect long time when lazy query enabled
…e when lazy query enabled (apache#1167) * KYLIN-4432 duplicated queries with sytax error take unexpect long time when lazy query enabled (cherry picked from commit 6342bc5)
…e when lazy query enabled (apache#1167) * KYLIN-4432 duplicated queries with sytax error take unexpect long time when lazy query enabled (cherry picked from commit 6342bc5)
Proposed changes
This PR fix the bug described in KYLIN-4432 that duplicated queries with sytax error take unexpect long time when lazy query enabled.
Types of changes
What types of changes does your code introduce to Kylin?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.document
branchFurther comments
N/A