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

Simplifies build by bundling Scribe (still disabled by default) #2576

Merged
merged 3 commits into from
May 10, 2019

Conversation

codefromthecrypt
Copy link
Member

We used to bundle scribe, but due to library conflicts, moved it
optional. As Armeria natively supports thrift (incidentally armeria
means sea thrift), we no longer have this problem.

Bundling removes one of the most complex things from our project for
sites that still use Scribe (most folks who use Finagle will).

This adds 1MiB to the server jar, which could be reduced further if
needed by:

  • inlining scribe types instead of using the large generated files
  • somehow removing the libthrift dep

I don't think this is worthwhile yet.

Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

LGTM with a small whisper

@codefromthecrypt
Copy link
Member Author

heh cool :)

Adrian Cole and others added 3 commits May 10, 2019 16:14
We used to bundle scribe, but due to library conflicts, moved it
optional. As Armeria natively supports thrift (incidentally armeria
means sea thrift), we no longer have this problem.

Bundling removes one of the most complex things from our project for
sites that still use Scribe (most folks who use Finagle will).
@codefromthecrypt codefromthecrypt merged commit 7f800e4 into master May 10, 2019
@codefromthecrypt codefromthecrypt deleted the bundle-scribe branch May 10, 2019 09:02
abesto pushed a commit to abesto/zipkin that referenced this pull request Sep 10, 2019
…zipkin#2576)

We used to bundle scribe, but due to library conflicts, moved it
optional. As Armeria natively supports thrift (incidentally armeria
means sea thrift), we no longer have this problem.

Bundling removes one of the most complex things from our project for
sites that still use Scribe (most folks who use Finagle will).
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.

2 participants