-
Notifications
You must be signed in to change notification settings - Fork 141
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
Add pit for join queries #2703
Add pit for join queries #2703
Changes from 14 commits
e225977
d69feda
b75d782
a336e3b
afd24d8
8f840ae
070b40f
58c96f5
3dad30d
1404cd5
fd09367
fcb584a
12f5abb
08a6a29
030f4b5
39f727a
be8d986
7bb89d8
60bd8fe
17d1d0e
3af87c4
bee4863
0857a6b
8e1c101
617d1db
841db6b
3185eda
2070f68
0bf9792
e13c50f
1366663
1408ccf
dd10d77
b487abf
f79d89e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.sql.legacy.pit; | ||
|
||
import lombok.Getter; | ||
import org.apache.logging.log4j.LogManager; | ||
import org.apache.logging.log4j.Logger; | ||
import org.opensearch.action.search.CreatePitRequest; | ||
import org.opensearch.action.search.CreatePitResponse; | ||
import org.opensearch.action.search.DeletePitRequest; | ||
import org.opensearch.client.Client; | ||
import org.opensearch.common.unit.TimeValue; | ||
import org.opensearch.core.action.ActionListener; | ||
|
||
public class PointInTimeHandler { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we write the classes to interface? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
private Client client; | ||
@Getter private String pitId; | ||
private static final Logger LOG = LogManager.getLogger(); | ||
|
||
public PointInTimeHandler(Client client, String[] indices) { | ||
this.client = client; | ||
|
||
CreatePitRequest createPitRequest = new CreatePitRequest(new TimeValue(600000), false, indices); | ||
rupal-bq marked this conversation as resolved.
Show resolved
Hide resolved
|
||
client.createPit( | ||
createPitRequest, | ||
new ActionListener<>() { | ||
@Override | ||
public void onResponse(CreatePitResponse createPitResponse) { | ||
pitId = createPitResponse.getId(); | ||
} | ||
|
||
@Override | ||
public void onFailure(Exception e) { | ||
LOG.error("Error occurred while creating PIT", e); | ||
} | ||
}); | ||
} | ||
|
||
public void deletePointInTime() { | ||
DeletePitRequest deletePitRequest = new DeletePitRequest(pitId); | ||
client.deletePits( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have synchronous calls? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no |
||
deletePitRequest, | ||
new ActionListener<>() { | ||
@Override | ||
public void onResponse(org.opensearch.action.search.DeletePitResponse deletePitResponse) { | ||
LOG.debug(deletePitResponse); | ||
} | ||
|
||
@Override | ||
public void onFailure(Exception e) { | ||
LOG.error("Error occurred while deleting PIT", e); | ||
} | ||
}); | ||
} | ||
} |
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.
So we're not switching to PIT completely but support both PIT and Scroll?
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.
Current plan is to add for all pagination then remove scroll after performance comparison.
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.
So we will raise another PR for this after performance test?
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.
yes I was planning to add search_after for all queries (join, multi query, pagination) then a separate PR to remove scroll after performance test.
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.
What if the PIT turns out be bad? We will leave setting to customer? What will be the default setting?
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.
Current default setting is true for using PIT with search after. After benchmark, we can decide if this needs to be changed. We can remove this while removing scroll.