Skip to content

SOLR-16236: Clarify use of GSON jar in jaegertracer-configurator#894

Merged
epugh merged 4 commits intoapache:mainfrom
epugh:SOLR-16236-clarify-gson-in-jaeger
Jul 6, 2022
Merged

SOLR-16236: Clarify use of GSON jar in jaegertracer-configurator#894
epugh merged 4 commits intoapache:mainfrom
epugh:SOLR-16236-clarify-gson-in-jaeger

Conversation

@epugh
Copy link
Copy Markdown
Contributor

@epugh epugh commented Jun 3, 2022

https://issues.apache.org/jira/browse/SOLR-16236

Description

README doesn't reflect whats actually going on... GSON is being included in the module, but shouldn't.

Solution

Fix readme! Fix gson being included.

Tests

manual builds

Checklist

Please review the following and check all that apply:

  • [X ] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • [ X] I have created a Jira issue and added the issue ID to my pull request title.
  • [ X] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • [ X] I have developed this patch against the main branch.
  • [ X] I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

@epugh epugh requested review from dsmiley and risdenk June 3, 2022 22:11
Copy link
Copy Markdown
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

Thanks

@risdenk
Copy link
Copy Markdown
Contributor

risdenk commented Jun 6, 2022

I'm not sure I understand why this change is needed. If "remote" is the default in jaeger, and "remote" requires gson - why are we excluding it? It just seems like the documentation was incorrect about needing to manually add gson. I don't think we should be excluding dependencies on a regular basis - especially if they are needed for things to work by default.

@epugh
Copy link
Copy Markdown
Contributor Author

epugh commented Jun 6, 2022

I'm not sure I understand why this change is needed. If "remote" is the default in jaeger, and "remote" requires gson - why are we excluding it? It just seems like the documentation was incorrect about needing to manually add gson. I don't think we should be excluding dependencies on a regular basis - especially if they are needed for things to work by default.

@dsmiley can you weigh in on this?

@dsmiley
Copy link
Copy Markdown
Contributor

dsmiley commented Jun 6, 2022

Check out the documentation of the sampler choices: https://www.jaegertracing.io/docs/1.22/sampling/
The remote choice is rather exotic IMO; I acknowledge it is nonetheless the default. We can more clearly document changing it, thus encouraging people to do so.
Nonetheless I don't feel too strongly about this. My preference is to reduce needless dependency exposure. But I see we already include it in some other modules (Tika, GCS, ...)

@epugh
Copy link
Copy Markdown
Contributor Author

epugh commented Jun 8, 2022

Thoughts folks? I kind of need a tie breaker to decide which way to go... I'd like to merge this sooner versus later as I'm accumulating more "pending PR's to get to done" then I like ;-) Maybe whoever updates the PR first wins? ;-)

@janhoy
Copy link
Copy Markdown
Contributor

janhoy commented Jun 8, 2022

I vote for keeping gson in the distro, making sure that even "remote" sampling works for all users. Seems quite useful in large deploys according to this blog: https://medium.com/jaegertracing/adaptive-sampling-in-jaeger-50f336f4334

@HoustonPutman
Copy link
Copy Markdown
Contributor

We already rely on it in other contribs, so I see no harm.

@epugh epugh requested review from HoustonPutman and janhoy July 6, 2022 15:12
@epugh
Copy link
Copy Markdown
Contributor Author

epugh commented Jul 6, 2022

Can I get a LGTM?

Copy link
Copy Markdown
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

👍

@epugh epugh merged commit 856d55b into apache:main Jul 6, 2022
epugh added a commit that referenced this pull request Jul 6, 2022
* consensus was to keep gson dependency, so update based on that.
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.

5 participants