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

[Feature][connectors-v2][kafka] Kafka supports custom schema #2371 #2783

Merged
merged 1 commit into from
Oct 19, 2022

Conversation

eyys
Copy link
Contributor

@eyys eyys commented Sep 18, 2022

Purpose of this pull request

[Feature][seatunnel-connectors-v2][connector-kafka] Kafka supports custom schema #2371
The last PR that was closed #2720

Check list

此拉取请求的目的

Kafka 支持自定义Schema #2371
上一个关闭的PR #2720

检查列表

添加e2e测试
添加example测试

Comment on lines 32 to 50
fields {
name = "string"
age = "int"
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks for the Pointers

}

sink {
Console {}
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines 46 to 61
sql {
sql = "select name,age from kafka"
}
Copy link
Member

Choose a reason for hiding this comment

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

remove or use select * from kafka selected all fields

Comment on lines 141 to 145
}else{
format = config.getString(FORMAT);
this.deserializationSchema = null;
}
}else {
Copy link
Member

Choose a reason for hiding this comment

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

check codestyle

}
}
topic = "test_csv"
bootstrap.server = "hadoop101:9092,hadoop102:9092,hadoop103:9092"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll try to set it up

result_table_name = "kafka"
schema = {
fields {
name = "string"
Copy link
Member

Choose a reason for hiding this comment

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

as above

}
}
topic = "test_csv"
bootstrap.server = "hadoop101:9092,hadoop102:9092,hadoop103:9092"
Copy link
Member

Choose a reason for hiding this comment

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

as above


transform {
sql {
sql = "select name,age from kafka"
Copy link
Member

Choose a reason for hiding this comment

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

as above


sink {
Console {}
}
Copy link
Member

Choose a reason for hiding this comment

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

as above

@eyys eyys requested review from hailin0 and removed request for hailin0 September 20, 2022 14:29
@hailin0
Copy link
Member

hailin0 commented Sep 24, 2022

recheck code

image

@eyys eyys force-pushed the develop2 branch 2 times, most recently from c4321a4 to 7f30f2a Compare September 25, 2022 15:38
@ashulin ashulin changed the title [Feature][seatunnel-connectors-v2][connector-kafka] Kafka supports custom schema #2371 [Feature][connectors-v2][kafka] Kafka supports custom schema #2371 Sep 28, 2022
Copy link
Member

@ashulin ashulin left a comment

Choose a reason for hiding this comment

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

Can you move e2e IT to seatunnel-connector-v2-e2e module? you can see #2924.

@eyys
Copy link
Contributor Author

eyys commented Sep 28, 2022

Can you move e2e IT to seatunnel-connector-v2-e2e module? you can see #2924.

Can I submit this first? The branches are a bit messy

@eyys eyys force-pushed the develop2 branch 4 times, most recently from 87c53d3 to aef883c Compare October 7, 2022 00:23
@eyys
Copy link
Contributor Author

eyys commented Oct 7, 2022

@ashulin @hailin0 Can you help review it

@eyys eyys force-pushed the develop2 branch 5 times, most recently from 0a8c1b3 to 9e804ca Compare October 10, 2022 14:43
@Hisoka-X
Copy link
Member

Please make sure CI passed first, thanks.

@eyys eyys force-pushed the develop2 branch 2 times, most recently from 6cdeaa6 to 0dafb56 Compare October 12, 2022 12:29
hailin0
hailin0 previously approved these changes Oct 13, 2022
Copy link
Member

@hailin0 hailin0 left a comment

Choose a reason for hiding this comment

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

LGTM

kafkaContainer = new KafkaContainer(DockerImageName.parse("confluentinc/cp-kafka:6.2.1"))
.withNetwork(NETWORK)
.withNetworkAliases(KAFKA_HOST)
.withLogConsumer(new Slf4jLogConsumer(log));
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
.withLogConsumer(new Slf4jLogConsumer(log));
.withLogConsumer(new Slf4jLogConsumer(DockerLoggerFactory.getLogger("confluentinc/cp-kafka:6.2.1")));

#3028

kafkaContainer = new KafkaContainer(DockerImageName.parse("confluentinc/cp-kafka:6.2.1"))
.withNetwork(NETWORK)
.withNetworkAliases(KAFKA_HOST)
.withLogConsumer(new Slf4jLogConsumer(log));
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
.withLogConsumer(new Slf4jLogConsumer(log));
.withLogConsumer(new Slf4jLogConsumer(DockerLoggerFactory.getLogger("confluentinc/cp-kafka:6.2.1")));

kafkaContainer = new KafkaContainer(DockerImageName.parse("confluentinc/cp-kafka:6.2.1"))
.withNetwork(NETWORK)
.withNetworkAliases(KAFKA_HOST)
.withLogConsumer(new Slf4jLogConsumer(log));
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
.withLogConsumer(new Slf4jLogConsumer(log));
.withLogConsumer(new Slf4jLogConsumer(DockerLoggerFactory.getLogger("confluentinc/cp-kafka:6.2.1")));

}
} else {
typeInfo = SeaTunnelSchema.buildSimpleTextSchema();
this.deserializationSchema = null;
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
this.deserializationSchema = null;
this.deserializationSchema = TextDeserializationSchema.builder()
.seaTunnelRowType(typeInfo)
.delimiter(String.valueOf('\002'))
.build();

If user does not assign schema option, so connector will treat data as the following shown:

content
xxxxxxxx

So the upstream data does not need to be delimited, so just pass in an impossible delimiter when initialization the TextDeserializationSchema.

Comment on lines 117 to 122
if (deserializationSchema != null) {
deserializationSchema.deserialize(record.value(), output);
} else {
String content = stringDeserializer.deserialize(partition.topic(), record.value());
output.collect(new SeaTunnelRow(new Object[]{content}));
}
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
if (deserializationSchema != null) {
deserializationSchema.deserialize(record.value(), output);
} else {
String content = stringDeserializer.deserialize(partition.topic(), record.value());
output.collect(new SeaTunnelRow(new Object[]{content}));
}
deserializationSchema.deserialize(record.value(), output)

TyrantLucifer
TyrantLucifer previously approved these changes Oct 17, 2022
Copy link
Member

@TyrantLucifer TyrantLucifer left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for your contribution. Let's waiting CI.

Hisoka-X
Hisoka-X previously approved these changes Oct 18, 2022
Copy link
Member

@Hisoka-X Hisoka-X left a comment

Choose a reason for hiding this comment

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

LGTM, waiting CI

Copy link
Member

@TyrantLucifer TyrantLucifer left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you for your contribution. CC @Hisoka-X

@Hisoka-X Hisoka-X merged commit 6506e30 into apache:dev Oct 19, 2022
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

5 participants