-
Notifications
You must be signed in to change notification settings - Fork 162
Pass JOIN_TIME_OUT value to keepalive #3826
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/executor/join/QueryPlanElasticExecutor.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/executor/join/QueryPlanElasticExecutor.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/executor/join/QueryPlanElasticExecutor.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImpl.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImpl.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/setting/OpenSearchSettings.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java
Outdated
Show resolved
Hide resolved
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.
Misclicked approve 😔
Signed-off-by: Kai Huang <[email protected]>
legacy/src/main/java/org/opensearch/sql/legacy/executor/join/QueryPlanElasticExecutor.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/query/planner/logical/node/TableScan.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Kai Huang <[email protected]>
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 for the changes! Just one last comment is UT / IT are both missing? If so, I think we should add both if possible.
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.
Please remove internal link in PR description.
+1 on test coverage |
legacy/src/main/java/org/opensearch/sql/legacy/query/planner/logical/node/TableScan.java
Outdated
Show resolved
Hide resolved
public void setCustomPitKeepAlive(TimeValue timeout) { | ||
this.customPitKeepAlive = Optional.ofNullable(timeout); | ||
LOG.info( | ||
"Config: Set custom PIT keepalive to: {}", | ||
customPitKeepAlive.map(t -> t + " (" + t.getMillis() + "ms)").orElse("default")); | ||
} | ||
|
||
/** | ||
* Check if custom PIT keepalive is set | ||
* | ||
* @return true if custom timeout is configured | ||
*/ | ||
public boolean hasCustomPitKeepAlive() { | ||
return customPitKeepAlive.isPresent(); | ||
} |
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.
- can we remove log on getter/setter?
- can we name methods consistently with the rest in this class?
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.
Removed methods
Pattern pattern = | ||
Pattern.compile( | ||
"/\\*!\\s*JOIN_TIME_OUT\\s*\\(\\s*(\\d+)\\s*\\)\\s*\\*/", Pattern.CASE_INSENSITIVE); | ||
Matcher matcher = pattern.matcher(sql); |
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.
Using regex is concerning on correctness and performance; can we reuse HintFactory
/ SqlParser
?
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.
Removed the use of regex and reused HintFactory/SqlParser
@@ -17,6 +20,7 @@ public class TableInJoinRequestBuilder { | |||
private List<Field> returnedFields; | |||
private Select originalSelect; | |||
private Integer hintLimit; | |||
private Config hintConfig; |
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.
Config has more fields than just hint. At this level this should be the same as hintLimit
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.
Removed hintConfig, using TimeValue hintJoinTimeout
import org.opensearch.transport.client.Client; | ||
|
||
/** Handler for Point In Time */ | ||
public class PointInTimeHandlerImpl implements PointInTimeHandler { | ||
private final Client client; | ||
private String[] indices; | ||
private final Optional<Config> config; |
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.
Config has more fields than just hint. We should reduce the scope
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.
Reduced scope to only TimeValue
legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java
Outdated
Show resolved
Hide resolved
request | ||
.getConfig() | ||
.getCustomPitKeepAlive() | ||
.map(timeout -> String.valueOf(timeout.getSeconds())) | ||
.orElse("unknown")); |
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.
why need this transformation here?
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.
Removed transformation
this.queryPlanner = request.plan(); | ||
} | ||
|
||
@Override | ||
public void run() throws IOException, SqlParseException { |
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.
why need to override the execution here?
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.
Removed the run() method
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
integ-test/src/test/java/org/opensearch/sql/legacy/JoinTimeoutHintIT.java
Outdated
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/legacy/JoinTimeoutHintIT.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
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 for the fix!
|
||
/** Consistency test: Verify repeated queries with JOIN_TIME_OUT produce consistent results */ | ||
@Test | ||
public void testJoinTimeoutHintConsistency() throws IOException { |
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.
This may become flaky because no deterministic guarantee either in index scan or limit operator?
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.
Removed test case, trimmed further
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Description
Pass JOIN_TIME_OUT value to keepalive
issue link
#3820
Result
When running this: