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

feat: create typsense plugin #2

Merged
merged 3 commits into from
Dec 9, 2024
Merged

feat: create typsense plugin #2

merged 3 commits into from
Dec 9, 2024

Conversation

nkwiatkowski
Copy link
Contributor

What changes are being made and why?

This is the initialisation of the typesense plugin. It allows to:

  • Index a document
  • get a document
  • Search documents
  • Search documents with facets
  • Index bulk of documents from an ION file

How the changes have been QAed?


Setup Instructions

collection: Countries
query: Paris
queryBy: capital
filter: countryName: [France, England]
Copy link
Member

Choose a reason for hiding this comment

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

How to apply multiple filter (if possible)?

Copy link
Contributor Author

@nkwiatkowski nkwiatkowski Dec 6, 2024

Choose a reason for hiding this comment

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

When using typescript, you can pass many filters in the same line -> price:>500 && in_stock:=true && category:='Electronics'
https://typesense.org/docs/26.0/api/search.html#filter-parameters

Copy link
Member

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

I didn't check the tests, I trust you on them ;)

Be careful, you often forget on 'e' in typesense!

build.gradle Outdated
@@ -30,7 +30,7 @@ java {
}

group "io.kestra.plugin"
description 'Plugin template for Kestra'
description 'Plugin typsense for Kestra'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description 'Plugin typsense for Kestra'
description 'Plugin typesense for Kestra'

build.gradle Outdated
@@ -98,6 +101,11 @@ dependencies {
testImplementation "org.junit.jupiter:junit-jupiter-engine"
testImplementation "org.hamcrest:hamcrest"
testImplementation "org.hamcrest:hamcrest-library"

//test container
testImplementation 'org.testcontainers:testcontainers:1.20.+'
Copy link
Member

Choose a reason for hiding this comment

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

We usually fix the latest version and let dependabot updating it.
We never use non-exact version.

@Schema(
title = "The host of the typsense DB",
example = "localhost",
requiredMode = RequiredMode.REQUIRED
Copy link
Member

Choose a reason for hiding this comment

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

requiredMode is not needed, we parse the annotation when generating the schema and add required=true if we found the @NotNull

protected Property<String> port;

@Schema(
title = "The api key to connect to the typsense DB",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title = "The api key to connect to the typsense DB",
title = "The API key to connect to the typsense DB",


@Schema(
title = "The name of the typsense collection",
example = "my_collection",
Copy link
Member

Choose a reason for hiding this comment

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

We don't use example so no need to add them.

private Property<String> from;

@Override
public VoidOutput run(RunContext runContext) throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

We usually retrun an output containing the number of line stored.

Comment on lines 67 to 69
} catch (ObjectNotFound e){
logger.error("No document found for id {}", renderedDocumentId, e);
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe let the task fail if there is nothing?
We usually do that.

Comment on lines 80 to 82
@Schema(
title = "Short description for this output",
description = "Full description of this output"
Copy link
Member

Choose a reason for hiding this comment

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

Hum, this didn't seems very accurate

.documents()
.upsert(runContext.render(document).asMap(String.class, Object.class));

runContext.metric(Counter.of("documentAdded", 1));
Copy link
Member

Choose a reason for hiding this comment

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

This seems useless, you have one doc you add a metric of 1, not very useful (but a metric would be great on the bulk task)

example = "countryName: [France, Germany]"
)
@Default
protected Property<String> filter = Property.of("");
Copy link
Member

Choose a reason for hiding this comment

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

default of empty seems not very usefull, maybe add no default as a property is always render via an Optional

Copy link
Member

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

LGTM, you can ignore my 2 small remarks and merge as is if you want.

title = "The chunk size for every bulk request",
description = "Default value is 1000"
)
private Property<Integer> chunk;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private Property<Integer> chunk;
private Property<Integer> chunk = Property.of(10);

This would correctly document the default value.

@nkwiatkowski nkwiatkowski reopened this Dec 9, 2024
@nkwiatkowski nkwiatkowski merged commit a59184d into master Dec 9, 2024
2 checks passed
@nkwiatkowski nkwiatkowski deleted the init_plugin branch December 9, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants