Skip to content

Refactory kafka pubsub components for code reuse#1695

Closed
skyao wants to merge 9 commits into
dapr:masterfrom
skyao:refactory-kafka-components
Closed

Refactory kafka pubsub components for code reuse#1695
skyao wants to merge 9 commits into
dapr:masterfrom
skyao:refactory-kafka-components

Conversation

@skyao

@skyao skyao commented Apr 29, 2022

Copy link
Copy Markdown
Member

Description

Refactory kafka pubsub components, extract the kafka common code from pubsub component and laster we can reuse them in kafka binding component.

This is just move code from kafka pubsub to internal/kafka and split the common kafka logic and pubsub component api, it should be safe from the view of functionality.

Issue reference

#1305

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

@skyao
skyao requested review from a team as code owners April 29, 2022 01:24

@shubham1172 shubham1172 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM overall - had a few questions.

Comment thread pubsub/kafka/kafka.go
Comment thread pubsub/kafka/kafka.go
return nil
type PubSub struct {
kafka *kafka.Kafka
topics map[string]bool

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need a map? Will []string not work?

Comment thread internal/component/kafka/auth.go
@codecov

codecov Bot commented May 10, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1695 (4ac9679) into master (bfd87eb) will increase coverage by 0.16%.
The diff coverage is 43.11%.

@@            Coverage Diff             @@
##           master    #1695      +/-   ##
==========================================
+ Coverage   36.37%   36.53%   +0.16%     
==========================================
  Files         166      173       +7     
  Lines       15488    15587      +99     
==========================================
+ Hits         5633     5694      +61     
- Misses       9228     9261      +33     
- Partials      627      632       +5     
Impacted Files Coverage Δ
bindings/azure/cosmosdb/cosmosdb.go 18.00% <ø> (ø)
bindings/cron/cron.go 87.23% <ø> (ø)
bindings/http/http.go 87.87% <ø> (ø)
bindings/kubernetes/kubernetes.go 17.14% <ø> (ø)
bindings/localstorage/localstorage.go 4.95% <0.00%> (-0.17%) ⬇️
internal/component/kafka/consumer.go 0.00% <0.00%> (ø)
internal/component/kafka/kafka.go 0.00% <0.00%> (ø)
internal/component/kafka/producer.go 0.00% <0.00%> (ø)
internal/component/kafka/sarama_log_bridge.go 0.00% <ø> (ø)
internal/component/kafka/sasl_oauthbearer.go 0.00% <ø> (ø)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 909d396...4ac9679. Read the comment docs.

@skyao

skyao commented May 11, 2022

Copy link
Copy Markdown
Member Author

This PR is included in PR

#1696

So close this one.

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.

3 participants