-
Notifications
You must be signed in to change notification settings - Fork 254
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 pipeline setup to index and create operations #587
Add pipeline setup to index and create operations #587
Conversation
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 @renatogm24. This is actually a bug, thanks for the catch and the fix!
For the fix to be complete, I added suggestions for requireAlias
which is also missing.
Would you mind also adding this test in BulkIngesterTest
?
@Test
public void pipelineTest() {
String json = "{\"create\":{\"_id\":\"some_id\",\"_index\":\"some_idx\",\"pipeline\":\"pipe\",\"require_alias\":true}}";
JsonpMapper mapper = new SimpleJsonpMapper();
BulkOperation create = BulkOperation.of(o -> o.create(c -> c
.pipeline("pipe")
.requireAlias(true)
.index("some_idx")
.id("some_id")
.document("Some doc")
));
String createStr = JsonpUtils.toJsonString(create, mapper);
assertEquals(json, createStr);
BulkOperation create1 = IngesterOperation.of(create, mapper).operation();
String create1Str = JsonpUtils.toJsonString(create1, mapper);
assertEquals(json, create1Str);
}
Once done, I'll be happy to merge this PR.
Fixes #586
@@ -154,6 +154,16 @@ private static void copyBaseProperties(BulkOperationBase op, BulkOperationBase.A | |||
.versionType(op.versionType()); | |||
} | |||
|
|||
private static void copyIndexProperties(IndexOperation<?> op, IndexOperation.Builder<?> builder) { | |||
copyBaseProperties(op, builder); | |||
builder.pipeline(op.pipeline()); |
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 you please add requireAlias
that is also part of index operations?
builder.pipeline(op.pipeline()); | |
builder.pipeline(op.pipeline()); | |
builder.requireAlias(op.requireAlias()); |
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 sure, it is done.
|
||
private static void copyCreateProperties(CreateOperation<?> op, CreateOperation.Builder<?> builder) { | ||
copyBaseProperties(op, builder); | ||
builder.pipeline(op.pipeline()); |
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.
Same here:
builder.pipeline(op.pipeline()); | |
builder.pipeline(op.pipeline()); | |
builder.requireAlias(op.requireAlias()); |
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 sure, it is done.
Hello @swallez, thanks for your response. I have just added the requireAlias to both copyProperties methods and also added the pipelineTest that you suggested, which is working. Thanks for your help. |
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.
LGTM. Thanks for the fix!
Co-authored-by: Renato <[email protected]>
Co-authored-by: Renato <[email protected]> Co-authored-by: Renato <[email protected]>
This pull request introduces a change to allow the pipeline in index operations to be set up individually. Previously, the pipeline was not set up for each index operation, which resulted in using the default pipeline on the index.
The changes include:
These changes allow each request to have its corresponding pipeline, providing more flexibility and control over how documents are indexed. This is particularly useful when different documents in the same index require different processing.
This change aligns with the existing structure of the BulkRequest, which also includes a "pipeline" field. With this change, the pipeline field in the BulkRequest will be correctly set up by the BulkIngester.