You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/06/03 22:11:34 UTC
[GitHub] [solr] epugh opened a new pull request, #894: SOLR-16236: Clarify use of GSON jar in jaegertracer-configurator
epugh opened a new pull request, #894:
URL: https://github.com/apache/solr/pull/894
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](https://wiki.apache.org/solr/HowToContribute) 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](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) 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](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #894: SOLR-16236: Clarify use of GSON jar in jaegertracer-configurator
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #894:
URL: https://github.com/apache/solr/pull/894#issuecomment-1149991915
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #894: SOLR-16236: Clarify use of GSON jar in jaegertracer-configurator
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #894:
URL: https://github.com/apache/solr/pull/894#issuecomment-1147506074
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #894: SOLR-16236: Clarify use of GSON jar in jaegertracer-configurator
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #894:
URL: https://github.com/apache/solr/pull/894#issuecomment-1147851975
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, ...)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #894: SOLR-16236: Clarify use of GSON jar in jaegertracer-configurator
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on PR #894:
URL: https://github.com/apache/solr/pull/894#issuecomment-1150044699
We already rely on it in other contribs, so I see no harm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh merged pull request #894: SOLR-16236: Clarify use of GSON jar in jaegertracer-configurator
Posted by GitBox <gi...@apache.org>.
epugh merged PR #894:
URL: https://github.com/apache/solr/pull/894
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #894: SOLR-16236: Clarify use of GSON jar in jaegertracer-configurator
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #894:
URL: https://github.com/apache/solr/pull/894#issuecomment-1149858265
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? ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #894: SOLR-16236: Clarify use of GSON jar in jaegertracer-configurator
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #894:
URL: https://github.com/apache/solr/pull/894#issuecomment-1147622724
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #894: SOLR-16236: Clarify use of GSON jar in jaegertracer-configurator
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #894:
URL: https://github.com/apache/solr/pull/894#issuecomment-1176345947
Can I get a LGTM?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org