Skip to content

Scripts for importing librdkafka static bundles #355

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

Merged
merged 4 commits into from
Apr 8, 2020
Merged

Conversation

edenhill
Copy link
Contributor

@edenhill edenhill commented Jul 15, 2019

Once this and the librdkafka PR has been reviewed, we'll make an additional PR with the actual librdkafka import.

The CI tests will not pass until the libs are imported.

README.md Outdated
If you use the master branch of the Go client, then you need to use the master branch of
librdkafka.
librdkafka is included with the Go client and does not have to be installed
separately on the build, nor target, system.
Copy link
Contributor

Choose a reason for hiding this comment

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

"and does not need to be installed separately on the build or target system" i think is better

README.md Outdated
separately on the build, nor target, system.


**Dynamic linking:** If you prefer to link librdkafka dynamically, which requires
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe: "If you prefer to link librdkafka dynamically you should build your application with go build -tags dynamic. This requires librdkafka to be installed on your system. ..."

Copy link
Contributor

@mhowlett mhowlett left a comment

Choose a reason for hiding this comment

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

this is well tested by now, right?

@edenhill edenhill force-pushed the selfstatic branch 2 times, most recently from 053d31f to d294038 Compare March 21, 2020 09:57
@mhowlett
Copy link
Contributor

Once this and the librdkafka PR has been reviewed, we'll make an additional PR with the actual librdkafka import.

are you planning on committing all librdkafka binary builds to the master branch over time (i.e. is that branch going to get very big over time)? I recall you were concerned about size increases if you made a mistake, but i don't understand the plan in the happy case.

i'm not a go practitioner and don't know the specifics, but i wanted to ask if this is an issue, and if there is anything that can be done about it, even if that is un-orthodox.

@edenhill
Copy link
Contributor Author

Each librdkafka import will be on its own branch that is merged, hopefully this will help in keeping the repo download size down when using shallow clones or snapshots, this is actually documented as part of this PR ;)
https://github.com/confluentinc/confluent-kafka-go/pull/355/files#diff-04fd5d79cdf652f9aeb3198432cfbe44R8

# and then later merged (NOT squashed or rebased).
# Having a merge per import allows future shallow clones to skip and ignore
# older imports, hopefully reducing the amount of git history data 'go get'
# needs to download.
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like shallow clone isn't going to be a default or option with go get any time soon: golang/go#13078 (comment)

(no actionable feedback here, more noting what i'm reading)

Copy link
Contributor Author

@edenhill edenhill Mar 27, 2020

Choose a reason for hiding this comment

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

Right, I think there are Go plans to use snapshots rather than clones in the future, which would be equivalent of --depth 1 minus the .git index (so even smaller).

git clone --depth 1 .. is still an option for people that highly care of import sizes.

I've done some explorative testing and these are the resulting numbers:

  • A standard librdkafka three platform build import is ~8MB per platform, a total of 24MB.
  • This consumes 24MB in the .gitindex and 24MB in the checkout itself = 48MB
  • Compressing the index (which is done by github and by manual git gc) brings the index size down to about 8MB
  • Importing 3 more releases, to a total of 4 releases, brings the index size up to 42MB, uncompressed.
  • Compressing that index again brings it down to 8MB again. This makes sense since these releases are nearly identical.
  • Performing a shallow clone (--depth 1) gives a clone that consumes 17MB: 8MB in the git index, 7MB in the checkout
  • A fresh full (no shallow) git get from github should thus transfer around 8MB and uncompress that to about 18MB.

@mhowlett
Copy link
Contributor

lg from my perspective. minor grammar suggestions

git checkout -b $import_branch

echo "Importing bundle $bundle"
./bundle-import.sh "$bundle" || error_cleanup

Choose a reason for hiding this comment

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

Why not just trap the error?

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 considered it, but I only wanted this cleanup for these specific errors 🤷‍♂

@edenhill edenhill force-pushed the selfstatic branch 5 times, most recently from 24ab482 to 5f942bb Compare March 27, 2020 15:06
This voids the need to install librdkafka separately.
@edenhill edenhill force-pushed the selfstatic branch 5 times, most recently from cfa77eb to 4246ab9 Compare April 8, 2020 13:32
edenhill added 3 commits April 8, 2020 15:39
…ency

Otherwise go_rdkafka_generr would still require librdkafka installed
separately, which is unnecessary now with librdkafka bundled.
@edenhill edenhill merged commit 1f6f5ba into master Apr 8, 2020
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