8000 [NU-2217] JSON parameter template instead of dynamic form for Kafka Sink by mateuszkp96 · Pull Request #8213 · TouK/nussknacker · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[NU-2217] JSON parameter template instead of dynamic form for Kafka Sink #8213

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 12 commits into
base: staging
Choose a base branch
from
Open

[NU-2217] JSON parameter template instead of dynamic form for Kafka Sink #8213

wants to merge 12 commits into from

Conversation

mateuszkp96
Copy link
Contributor
@mateuszkp96 mateuszkp96 commented Jun 9, 2025 •
edited
Loading

Describe your changes

image

Checklist before merge

  • Related issue ID is placed at the beginning of PR title in [brackets] (can be GH issue or Nu Jira issue)
  • Code is cleaned from temporary changes and commented out lines
  • Parts of the code that are not easy to understand are documented in the code
  • Changes are covered by automated tests
  • Showcase in dev-application.conf added to demonstrate the feature
  • Documentation added or updated
  • Added entry in Changelog.md describing the change from the perspective of a public distribution user
  • Added MigrationGuide.md entry in the appropriate subcategory if introducing a breaking change
  • Verify that PR will be squashed during merge

@github-actions github-actions bot added the docs label Jun 9, 2025
@github-actions github-actions bot mentioned this pull request Jun 9, 2025
Closed
@github-actions GitHub Actions
Copy link
Contributor
github-actions bot commented Jun 9, 2025 •
edited
Loading

created: #8268
⚠️ Be careful! Snapshot changes are not necessarily the cause of the error. Check the logs.

@mateuszkp96 mateuszkp96 force-pushed the NU-2217-param-template-instead-of-dynamic-form branch 2 times, most recently from 2043baa to 6b33bf5 Compare June 16, 2025 05:55
arkadius
arkadius reviewed Jun 17, 2025
View reviewed changes
...er/engine/schemedkafka/schemaregistry/confluent/client/MockSchemaRegistryClientBuilder.scala Outdated
new RegistryItem(topic, AvroUtils.parseSchema(schema), version, isKey, AutoIncId)
new RegistryItem(topic, toParsedSchema(AvroUtils.parseSchema(schema), version), version, isKey, AutoIncId)

def apply(topic: String, schema: Schema, version: Int, isKey: Boolean): RegistryItem = {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like, RegistryItem.apply is used only once. I think that someone, before you, overengineered this API, preparing it for many usages in many different cases. Let's simplify this class by removing all apply methods and discuss the API which is more commolny used (MockConfluentSchemaRegistryClientBuilder.register)

.../src/main/scala/pl/touk/nussknacker/engine/schemedkafka/sink/UniversalKafkaSinkFactory.scala Outdated
)
}

protected def handleTopicWithSchemaWithJsonTemplateEditor(
Copy link
Member

Choose a reason for hiding this comment

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

Are you still getting what's going on in this class? Because I'm not :) What does WithJsonTemplateEditor mean in this context? Does this method handle configuration option when there is a single parameter for value? I see that this pattern matching is enabled even if this option is not turned on. WDYT about having two PartialFunction for both options and using one of them depending on configuration option? Will it decompose the problem? Or maybe at least, we could extract some parts of this pattern matching to dedicated classes that would name the scope of this pattern patching that they solve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simplified it a little bit. Please take a look

@mateuszkp96 mateuszkp96 requested a review from arkadius June 23, 2025 10:16
arkadius
arkadius reviewed Jun 24, 2025
View reviewed changes
.../scala/pl/touk/nussknacker/engine/schemedkafka/typed/AvroSchemaTypeDefinitionExtractor.scala Outdated
@@ -33,6 +33,7 @@ class AvroSchemaTypeDefinitionExtractor(recordUnderlyingType: TypedClass) {
case Schema.Type.RECORD => {
val fields = schema.getFields.asScala
.map(field => field.name() -> typeDefinition(field.schema()))
.sortBy(_._1)
Copy link
Member

Choose a reason for hiding this comment

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

As we discussed, for Avro, we should keep the original order of fields

arkadius
arkadius reviewed Jun 24, 2025
View reviewed changes
...ponents-utils/src/test/scala/pl/touk/nussknacker/engine/schemedkafka/schema/TestSchema.scala Outdated

trait TestSchema {
lazy val schema: Schema = AvroUtils.parseSchema(stringSchema)
lazy val schema: Schema = AvroUtils.parseSchema(stringSchema)
lazy val confluentSchema: AvroSchema = ConfluentUtils.convertToAvroSchema(schema)
Copy link
Member

Choose a reason for hiding this comment

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

Having two schemas next to each other is confusing. I thought about an approach, that we keep in samples this most commonly know type for schemas, which is easiest to build using DSL and to parse - org.apache.avro.Schema, and we pass it to MockConfluentSchemaRegistryClientBuilder.register where we convert it to ParsedSchema just for MockSchemaRegistryClient.register purpose.

arkadius
arkadius reviewed Jun 24, 2025
View reviewed changes
.../src/main/scala/pl/touk/nussknacker/engine/schemedkafka/sink/UniversalKafkaSinkFactory.scala Outdated
case Some(validationModeString) => extractValidationMode(validationModeString)
case None => ValidationMode.strict
val validationMode = {
if (params.isPresent(sinkRawEditorParamName)) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we can use just extract without Unsafe and avoid direct isPresent calling

.../src/main/scala/pl/touk/nussknacker/engine/schemedkafka/sink/UniversalKafkaSinkFactory.scala Outdated
} else if (params.isPresent(sinkValidationModeParamName)) {
validationModeParamDeclaration.extractValue(params) match {
case Some(validationModeString) => extractValidationMode(validationModeString)
case None => ValidationMode.strict
Copy link
Member

Choose a reason for hiding this comment

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

This is a dead code. If you checked isPresent, extractValue will always return true.

.../src/main/scala/pl/touk/nussknacker/engine/schemedkafka/sink/UniversalKafkaSinkFactory.scala Outdated
if (params.extractUnsafe[Boolean](sinkRawEditorParamName)) {
validationModeParamDeclaration.extractValue(params) match {
case Some(validationModeString) => extractValidationMode(validationModeString)
case None => ValidationMode.strict
Copy link
Member

Choose a reason for hiding this comment

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

I thought that for raw editor, validation mode is alywas avialable. Isn't it? It is for schemaless case? If so, lets' write a comment that this value is not important

@mateuszkp96 mateuszkp96 force-pushed the NU-2217-param-template-instead-of-dynamic-form branch from c211ba4 to c35d62f Compare June 24, 2025 13:36
@github-actions github-actions bot mentioned this pull request Jun 24, 2025
Closed
Sign up for free to join this conversation on GitHub. Alrea 606D dy have an account? Sign in to comment
Reviewers

@arkadius arkadius arkadius left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mateuszkp96 @arkadius
0