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 2021/06/04 11:11:34 UTC
[GitHub] [solr] cpoerschke opened a new pull request #162: SOLR-14920: contrib/jaegertracer-configurator: apply & enforce 'spotless' code formatting
cpoerschke opened a new pull request #162:
URL: https://github.com/apache/solr/pull/162
https://issues.apache.org/jira/browse/SOLR-14920
Building upon #126 which added spotless support without use of it.
- [X] pull request branch is up to date with latest `origin/main`
- [X] `./gradlew -p solr/contrib/jaegertracer-configurator test` passes
--
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.
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] cpoerschke commented on a change in pull request #162: SOLR-14920: contrib/jaegertracer-configurator: apply & enforce 'spotless' code formatting
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #162:
URL: https://github.com/apache/solr/pull/162#discussion_r646906653
##########
File path: solr/contrib/jaegertracer-configurator/src/test/org/apache/solr/jaeger/TestJaegerConfigurator.java
##########
@@ -53,26 +51,23 @@ public void doBefore() {
@Test
public void testInjected() throws Exception {
- MiniSolrCloudCluster cluster = new SolrCloudTestCase.Builder(2, createTempDir())
- .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf"))
- .withSolrXml(getFile("solr/solr.xml").toPath())
- .build();
+ MiniSolrCloudCluster cluster =
+ new SolrCloudTestCase.Builder(2, createTempDir())
+ .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf"))
+ .withSolrXml(getFile("solr/solr.xml").toPath())
+ .build();
try {
TimeOut timeOut = new TimeOut(2, TimeUnit.MINUTES, TimeSource.NANO_TIME);
timeOut.waitFor(
"Waiting for GlobalTracer is registered",
() -> GlobalTracer.get().toString().contains("JaegerTracer"));
- //TODO add run Jaeger through Docker and verify spans available after run these commands
+ // TODO add run Jaeger through Docker and verify spans available after run these commands
CollectionAdminRequest.createCollection("test", 2, 1).process(cluster.getSolrClient());
- new UpdateRequest()
- .add("id", "1")
- .add("id", "2")
- .process(cluster.getSolrClient(), "test");
+ new UpdateRequest().add("id", "1").add("id", "2").process(cluster.getSolrClient(), "test");
Review comment:
> ... How are the rules configured for overrides? ... I'd like to understand more about the plugin we're applying.
From what I understand https://github.com/apache/solr/blob/a9a8d2023de00474277c6edbaff75d8bd12f33e8/gradle/validation/spotless.gradle#L37-L41 is the area where the configuration happens and as per https://github.com/diffplug/spotless/tree/main/plugin-gradle there are many options available (though from a little bit of "browsing around" i couldn't find one that obviously covers the situation here).
Oh and please note that `spotless.gradle` is also being changed as part of this pull request (to remove the exemption for `contrib/jaegertracer-configurator`) but the github UI wouldn't let me add a comment on the relevant lines, hence commenting here where the question cropped up.
##########
File path: solr/contrib/jaegertracer-configurator/src/test/org/apache/solr/jaeger/TestJaegerConfigurator.java
##########
@@ -37,8 +36,7 @@
public class TestJaegerConfigurator extends SolrTestCaseJ4 {
- @Rule
- public TestRule solrTestRules = new SystemPropertiesRestoreRule();
+ @Rule public TestRule solrTestRules = new SystemPropertiesRestoreRule();
Review comment:
> Is this option configurable?
Experimentally if `solrTestRules` is changed to be a different name so that the line with the `@Rule` prepended is quite a bit longer then it leaves the `@Rule` on the previous line.
I also note that https://google.github.io/styleguide/javaguide.html#s4.8.5-annotations says "A _single_ parameterless annotation _may_ instead appear together with the first line of the signature" i.e. "_may_" not "_must_" and that the Lucene code with the same configuration has a mix of prior and same line `@Rule` style.
--
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.
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] cpoerschke commented on a change in pull request #162: SOLR-14920: contrib/jaegertracer-configurator: apply & enforce 'spotless' code formatting
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #162:
URL: https://github.com/apache/solr/pull/162#discussion_r646906653
##########
File path: solr/contrib/jaegertracer-configurator/src/test/org/apache/solr/jaeger/TestJaegerConfigurator.java
##########
@@ -53,26 +51,23 @@ public void doBefore() {
@Test
public void testInjected() throws Exception {
- MiniSolrCloudCluster cluster = new SolrCloudTestCase.Builder(2, createTempDir())
- .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf"))
- .withSolrXml(getFile("solr/solr.xml").toPath())
- .build();
+ MiniSolrCloudCluster cluster =
+ new SolrCloudTestCase.Builder(2, createTempDir())
+ .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf"))
+ .withSolrXml(getFile("solr/solr.xml").toPath())
+ .build();
try {
TimeOut timeOut = new TimeOut(2, TimeUnit.MINUTES, TimeSource.NANO_TIME);
timeOut.waitFor(
"Waiting for GlobalTracer is registered",
() -> GlobalTracer.get().toString().contains("JaegerTracer"));
- //TODO add run Jaeger through Docker and verify spans available after run these commands
+ // TODO add run Jaeger through Docker and verify spans available after run these commands
CollectionAdminRequest.createCollection("test", 2, 1).process(cluster.getSolrClient());
- new UpdateRequest()
- .add("id", "1")
- .add("id", "2")
- .process(cluster.getSolrClient(), "test");
+ new UpdateRequest().add("id", "1").add("id", "2").process(cluster.getSolrClient(), "test");
Review comment:
> ... How are the rules configured for overrides? ... I'd like to understand more about the plugin we're applying.
From what I understand https://github.com/apache/solr/blob/a9a8d2023de00474277c6edbaff75d8bd12f33e8/gradle/validation/spotless.gradle#L37-L41 (added in #126) is the area where the configuration happens and as per https://github.com/diffplug/spotless/tree/main/plugin-gradle there are many options available (though from a little bit of "browsing around" i couldn't find one that obviously covers the situation here).
Oh and please note that `spotless.gradle` is also being changed as part of this pull request (to remove the exemption for `contrib/jaegertracer-configurator`) but the github UI wouldn't let me add a comment on the relevant lines, hence commenting here where the question cropped up.
--
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.
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] madrob commented on a change in pull request #162: SOLR-14920: contrib/jaegertracer-configurator: apply & enforce 'spotless' code formatting
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #162:
URL: https://github.com/apache/solr/pull/162#discussion_r646774159
##########
File path: solr/contrib/jaegertracer-configurator/src/test/org/apache/solr/jaeger/TestJaegerConfigurator.java
##########
@@ -37,8 +36,7 @@
public class TestJaegerConfigurator extends SolrTestCaseJ4 {
- @Rule
- public TestRule solrTestRules = new SystemPropertiesRestoreRule();
+ @Rule public TestRule solrTestRules = new SystemPropertiesRestoreRule();
Review comment:
Is this option configurable?
##########
File path: solr/contrib/jaegertracer-configurator/src/test/org/apache/solr/jaeger/TestJaegerConfigurator.java
##########
@@ -53,26 +51,23 @@ public void doBefore() {
@Test
public void testInjected() throws Exception {
- MiniSolrCloudCluster cluster = new SolrCloudTestCase.Builder(2, createTempDir())
- .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf"))
- .withSolrXml(getFile("solr/solr.xml").toPath())
- .build();
+ MiniSolrCloudCluster cluster =
+ new SolrCloudTestCase.Builder(2, createTempDir())
+ .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf"))
+ .withSolrXml(getFile("solr/solr.xml").toPath())
+ .build();
try {
TimeOut timeOut = new TimeOut(2, TimeUnit.MINUTES, TimeSource.NANO_TIME);
timeOut.waitFor(
"Waiting for GlobalTracer is registered",
() -> GlobalTracer.get().toString().contains("JaegerTracer"));
- //TODO add run Jaeger through Docker and verify spans available after run these commands
+ // TODO add run Jaeger through Docker and verify spans available after run these commands
CollectionAdminRequest.createCollection("test", 2, 1).process(cluster.getSolrClient());
- new UpdateRequest()
- .add("id", "1")
- .add("id", "2")
- .process(cluster.getSolrClient(), "test");
+ new UpdateRequest().add("id", "1").add("id", "2").process(cluster.getSolrClient(), "test");
Review comment:
I'm not sure I agree with this reformatting. How are the rules configured for overrides? This is a minor nit, and if it's the only issue then I'll deal with all of it on one line, but I'd like to understand more about the plugin we're applying.
--
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.
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] madrob commented on a change in pull request #162: SOLR-14920: contrib/jaegertracer-configurator: apply & enforce 'spotless' code formatting
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #162:
URL: https://github.com/apache/solr/pull/162#discussion_r648608776
##########
File path: solr/contrib/jaegertracer-configurator/src/test/org/apache/solr/jaeger/TestJaegerConfigurator.java
##########
@@ -37,8 +36,7 @@
public class TestJaegerConfigurator extends SolrTestCaseJ4 {
- @Rule
- public TestRule solrTestRules = new SystemPropertiesRestoreRule();
+ @Rule public TestRule solrTestRules = new SystemPropertiesRestoreRule();
Review comment:
Thanks for researching this, I think at the end of the day I don't care what the rule is as long as it is easy to enforce and validate.
--
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.
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] cpoerschke commented on a change in pull request #162: SOLR-14920: contrib/jaegertracer-configurator: apply & enforce 'spotless' code formatting
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #162:
URL: https://github.com/apache/solr/pull/162#discussion_r650094150
##########
File path: solr/contrib/jaegertracer-configurator/src/java/org/apache/solr/jaeger/JaegerTracerConfigurator.java
##########
@@ -31,7 +31,5 @@ public Tracer getTracer() {
}
@Override
- public void init(@SuppressWarnings({"rawtypes"})NamedList args) {
- }
-
+ public void init(@SuppressWarnings({"rawtypes"}) NamedList args) {}
Review comment:
> This conflict with one of the changes I made, it's easy to resolve though, just delete everything!
Good catch! I assume #169 are the other changes. Would you like to proceed with that pull request first or would you prefer this PR here be merged first? Either approach would work for me.
--
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.
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 #162: SOLR-14920: contrib/jaegertracer-configurator: apply & enforce 'spotless' code formatting
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #162:
URL: https://github.com/apache/solr/pull/162#issuecomment-854778916
You could increase the scope of the PR and just keep going with more modules, thus ultimately fewer commits where we make changes en-masse. Up to you.
Once the merges happen, the commits where we do this en-masse change should be added to `/.git-blame-ignore-revs`
The current contents of that file should be wiped clean because they are all Lucene commits. We inherited it from the split.
--
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.
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] madrob commented on a change in pull request #162: SOLR-14920: contrib/jaegertracer-configurator: apply & enforce 'spotless' code formatting
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #162:
URL: https://github.com/apache/solr/pull/162#discussion_r651146459
##########
File path: solr/contrib/jaegertracer-configurator/src/java/org/apache/solr/jaeger/JaegerTracerConfigurator.java
##########
@@ -31,7 +31,5 @@ public Tracer getTracer() {
}
@Override
- public void init(@SuppressWarnings({"rawtypes"})NamedList args) {
- }
-
+ public void init(@SuppressWarnings({"rawtypes"}) NamedList args) {}
Review comment:
No, it was #165, once you resolve the conflict here then this should be good to merge!
--
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.
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] cpoerschke merged pull request #162: SOLR-14920: contrib/jaegertracer-configurator: apply & enforce 'spotless' code formatting
Posted by GitBox <gi...@apache.org>.
cpoerschke merged pull request #162:
URL: https://github.com/apache/solr/pull/162
--
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.
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] cpoerschke commented on pull request #162: SOLR-14920: contrib/jaegertracer-configurator: apply & enforce 'spotless' code formatting
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #162:
URL: https://github.com/apache/solr/pull/162#issuecomment-854884550
> Once the merges happen, the commits where we do this en-masse change should be added to `/.git-blame-ignore-revs`
> The current contents of that file should be wiped clean because they are all Lucene commits. We inherited it from the split.
Thanks, I didn't know about that file! Created #163 for the wiping clean separately first.
--
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.
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] madrob commented on a change in pull request #162: SOLR-14920: contrib/jaegertracer-configurator: apply & enforce 'spotless' code formatting
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #162:
URL: https://github.com/apache/solr/pull/162#discussion_r648609924
##########
File path: solr/contrib/jaegertracer-configurator/src/java/org/apache/solr/jaeger/JaegerTracerConfigurator.java
##########
@@ -31,7 +31,5 @@ public Tracer getTracer() {
}
@Override
- public void init(@SuppressWarnings({"rawtypes"})NamedList args) {
- }
-
+ public void init(@SuppressWarnings({"rawtypes"}) NamedList args) {}
Review comment:
This conflict with one of the changes I made, it's easy to resolve though, just delete everything!
--
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.
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