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

Upgrade to Pulsar 3.0.1 (BATCH-318) #2555

Closed
wants to merge 14 commits into from

Conversation

richscott
Copy link
Member

@richscott richscott commented Jun 12, 2023

Update Pulsar images to version 3.0.0. That version now has direct support for ARM64 images, so remove references to locally-built ones. Update pulsar-client Go library to latest version (v0.10.0).

Also, change a rmdir invocation to rm -r in e2e test target of makefile, in case the .kube directory is not already empty.

Some mocks had to be regenerated (via make generate target) as the new pulsar-client-go version now has Consumer.Ack(Message) and Consumer.AckID(msgID) returning an error.

All unit tests, plus the tests-e2e and tests-e2e-airflow integration tests pass successfully, with these changes.

These changes won't be merged until approved and/or tested by GR devs.

Fixes https://gr-oss.atlassian.net/browse/BATCH-318

┆Issue is synchronized with this Jira Task by Unito

Update images to Pulsar 3.0.0. That version now has direct support for
ARM64 images, so remove references to locally-built ones. Update
pulsar-client Go library to latest version. Also, change a `rmdir`
invocation to `rm -r` in e2e test target of makefile, in case the .kube
directory is not already empty.
@richscott richscott added the do-not-merge Do not merge this PR label Jun 12, 2023
@richscott richscott self-assigned this Jun 12, 2023
Update images to Pulsar 3.0.0. That version now has direct support for
ARM64 images, so remove references to locally-built ones. Update
pulsar-client Go library to latest version. Also, change a `rmdir`
invocation to `rm -r` in e2e test target of makefile, in case the .kube
directory is not already empty.
@Sharpz7
Copy link
Contributor

Sharpz7 commented Jun 12, 2023

I see no problems with this, outside of changing the go.mod. I found that doing that broke the linting completely as well when I tried it with goreleaser.

@richscott richscott changed the title Upgrade to Pulsar 3.0.0 (BATCH-318) Draft: Upgrade to Pulsar 3.0.0 (BATCH-318) Jun 12, 2023
@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Patch coverage: 8.33% and project coverage change: -2.34% ⚠️

Comparison is base (19a6858) 46.95% compared to head (bba1960) 44.61%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2555      +/-   ##
==========================================
- Coverage   46.95%   44.61%   -2.34%     
==========================================
  Files         392      373      -19     
  Lines       43041    39013    -4028     
  Branches      487      487              
==========================================
- Hits        20208    17404    -2804     
+ Misses      21319    20363     -956     
+ Partials     1514     1246     -268     
Flag Coverage Δ
unittests 44.61% <8.33%> (-2.34%) ⬇️

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

Files Changed Coverage Δ
internal/armada/server/eventsprinter.go 0.00% <0.00%> (ø)
internal/armada/server/submit_from_log.go 4.12% <0.00%> (-0.08%) ⬇️
internal/common/ingest/ingestion_pipeline.go 54.48% <25.00%> (-1.47%) ⬇️
internal/common/pulsarutils/async.go 58.57% <25.00%> (-2.63%) ⬇️

... and 19 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sharpz7
Copy link
Contributor

Sharpz7 commented Jun 13, 2023

Can you add in the new image here as well? https://github.com/armadaproject/armada/blob/master/magefiles/developer.go#L32

@richscott richscott changed the title Draft: Upgrade to Pulsar 3.0.0 (BATCH-318) Upgrade to Pulsar 3.0.0 (BATCH-318) Jun 15, 2023
@@ -9,6 +9,6 @@ import (
const LookoutSql = "lookout/sql" // static asset namespace

func init() {
Copy link
Contributor

Choose a reason for hiding this comment

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

From this file and below, do you know why these were included? Looks like we committed code unrelated to pulsar upgrade as I hope lookout is unrelated to Pulsar.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree - Lookout is unrelated to Pulsar (as far as I know). When I did the Pulsar 3 upgrade, I had to upgrade the pulsar-client-go library (see the (very few) changes in go.mod), and they changed the signature on about 3 functions to start returning an error (when they returned nothing before). So I fixed our interface definitions that needed those changes, which required regenerating the mocks - that last step caused all the changes in the lookout/lookoutv2 dirs, including that giant statik definition. I thought about seeing if I could revert some of the files that were changed by generate, but compilation and testing quickly failed, so I just stayed with clean generated updates (I did no manual tweaks to those files after running the generate step).

kannon92
kannon92 previously approved these changes Jun 22, 2023
@richscott richscott changed the title Upgrade to Pulsar 3.0.0 (BATCH-318) Upgrade to Pulsar 3.0.1 (BATCH-318) Aug 16, 2023
@richscott richscott closed this Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Do not merge this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants