Skip to content
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

Conversation

renatogm24
Copy link
Contributor

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:

  • Adding a copyIndexProperties method to copy the pipeline property from the IndexOperation to the IndexOperation.Builder.
  • Modifying the indexOperation method to use copyIndexProperties instead of copyBaseProperties, ensuring the pipeline is copied for each index operation.
  • Similar changes have been made for createOperation method to ensure consistency.

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.

Copy link
Member

@swallez swallez left a 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());
Copy link
Member

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?

Suggested change
builder.pipeline(op.pipeline());
builder.pipeline(op.pipeline());
builder.requireAlias(op.requireAlias());

Copy link
Contributor Author

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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here:

Suggested change
builder.pipeline(op.pipeline());
builder.pipeline(op.pipeline());
builder.requireAlias(op.requireAlias());

Copy link
Contributor Author

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.

@renatogm24
Copy link
Contributor Author

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.

@renatogm24 renatogm24 requested a review from swallez June 9, 2023 18:08
Copy link
Member

@swallez swallez left a 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!

@swallez swallez merged commit 38ec037 into elastic:main Jun 13, 2023
3 checks passed
swallez pushed a commit that referenced this pull request Jun 13, 2023
Co-authored-by: Renato <Renato.Garay.-ND@disney.com>
swallez added a commit that referenced this pull request Jun 13, 2023
Co-authored-by: Renato <renato.garay@pucp.pe>
Co-authored-by: Renato <Renato.Garay.-ND@disney.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants