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.

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

Deprecate kafka output and move the code to an extension #1917

Merged
merged 1 commit into from
Apr 2, 2021
Merged

Deprecate kafka output and move the code to an extension #1917

merged 1 commit into from
Apr 2, 2021

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Mar 18, 2021 •
edited
Loading

fixes #1934

na--
na-- reviewed Mar 22, 2021
View reviewed changes
cmd/outputs.go Outdated Show resolved Hide resolved
@mstoykov mstoykov mentioned this pull request Mar 29, 2021
Closed
@na-- na-- mentioned this pull request Mar 29, 2021
Closed
imiric
imiric reviewed Mar 30, 2021
View reviewed changes
Copy link
Contributor

@imiric imiric left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Certainly happy to see those indirect dependencies go :)

Also, it would be great to have E2E/integration tests with an actual Kafka instance. We could set it up in the extension repo eventually.

cmd/outputs.go Outdated Show resolved Hide resolved
@mstoykov mstoykov force-pushed the dropKafkaFromMainCode branch from 27d1787 to 9689565 Compare March 31, 2021 15:37
@mstoykov
Copy link
Contributor Author

mstoykov commented Apr 1, 2021

I would prefer if we merge this after #1940 as I will need to rebase one or the other and this one is just ... smaller :D

@mstoykov mstoykov requested review from imiric and na-- April 1, 2021 07:02
@mstoykov mstoykov changed the title Get kafka output out of extension Deprecate kafka output and move the code to an extension Apr 1, 2021
@mstoykov mstoykov force-pushed the dropKafkaFromMainCode branch from 2504ac0 to 4ac4660 Compare April 1, 2021 08:45
imiric
imiric reviewed Apr 1, 2021
View reviewed changes
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Haven't tested it, but LGTM.

cmd/outputs.go Outdated Show resolved Hide resolved
imiric
imiric previously approved these changes Apr 1, 2021
View reviewed changes
@mstoykov
Copy link
Contributor Author

mstoykov commented Apr 1, 2021

Haven't tested it, but LGTM.

I have tested it so far as I went through the blog about it and with both cli and env variables configuration I was seeing metrics. I doubt we broke the format of them or something like that so this should be enough ?

na--
na-- reviewed Apr 2, 2021
View reviewed changes
cmd/outputs.go Outdated Show resolved Hide resolved
na--
na-- previously approved these changes Apr 2, 2021
View reviewed changes
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

LGTM besides the minor message correction.

@mstoykov mstoykov dismissed stale reviews from na-- and imiric via 67118b1 April 2, 2021 14:19
@codecov-io
Copy link

Codecov Report

Merging #1917 (8658c54) into master (259982a) will increase coverage by 0.12%.
The diff coverage is 0.00%.

❗ Current head 8658c54 differs from pull request most recent head 67118b1. Consider uploading reports for the commit 67118b1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1917      +/-   ##
==========================================
+ Coverage   71.21%   71.34%   +0.12%     
==========================================
  Files         183      181       -2     
  Lines       14336    14219     -117     
==========================================
- Hits        10210    10144      -66     
+ Misses       3486     3461      -25     
+ Partials      640      614      -26
Flag Coverage Δ
ubuntu 71.30% <0.00%> (+0.12%) ⬆️
windows 70.98% <0.00%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/outputs.go 0.00% <0.00%> (ø)
stats/influxdb/config.go 33.62% <0.00%> (-4.32%) ⬇️
stats/influxdb/util.go 78.72% <0.00%> (-4.26%) ⬇️
stats/influxdb/collector.go 77.57% <0.00%> (-3.74%) ⬇️
lib/executor/vu_handle.go 95.23% <0.00%> (+0.95%) ⬆️
lib/testutils/minirunner/minirunner.go 80.43% <0.00%> (+4.34%) ⬆️

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 259982a...67118b1. Read the comment docs.

na--
na-- previously approved these changes Apr 2, 2021
View reviewed changes
@mstoykov mstoykov force-pushed the dropKafkaFromMainCode branch from 67118b1 to fee8019 Compare April 2, 2021 14:50
@mstoykov mstoykov requested review from imiric and na-- April 2, 2021 14:51
@mstoykov mstoykov force-pushed the dropKafkaFromMainCode branch from fee8019 to 3f47870 Compare April 2, 2021 14:58
@mstoykov mstoykov merged commit 4d90596 into master Apr 2, 2021
@mstoykov mstoykov deleted the dropKafkaFromMainCode branch April 2, 2021 15:12
@na-- na-- added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Apr 2, 2021
@na-- na-- added this to the v0.32.0 milestone Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@na-- na-- na-- approved these changes

@imiric imiric Awaiting requested review from imiric

Assignees
No one assigned
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes
Projects
None yet
Milestone
v0.32.0
Development

Successfully merging this pull request may close these issues.

Deprecate the built-in kafka output and shift to an xk6 extension
4 participants
@mstoykov @codecov-io @imiric @na--