You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "renatoh (via GitHub)" <gi...@apache.org> on 2023/04/16 09:13:26 UTC
[GitHub] [solr] renatoh opened a new pull request, #1570: Move duplicated code to method in TaskManagementHandler.java
renatoh opened a new pull request, #1570:
URL: https://github.com/apache/solr/pull/1570
small refactoring to get rid of duplicated code
--
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 #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1529040695
I actually want to look a bit more at the method name... And if it should actually be on the SearchComponent or elsewhere... it isn't (I don't think!) specific to TaskManagement???
--
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] renatoh commented on pull request #1570: Factor out ModifiableSolrParams.setShardAttributesToParams method
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1546628615
@risdenk Could you please have a look at it any time soon, thanks
--
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 #1570: Factor out ModifiableSolrParams.setShardAttributesToParams method
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1557311008
Going to merge today once checks run!
--
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 #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1529040022
sometimes "crave" get's in a odd state.. well, I should say "frequently" with how it merges stuf.... I see the precommit passed. How would you like to credited in the CHANGES.txt file?
--
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] renatoh commented on pull request #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1529036040
In the log I see:
error: patch failed: gradlew.bat:80
error: gradlew.bat: patch does not apply
Not sure what this means, but given the checks were successful last week I assume it is because my branch got out of sync with main-branch thus I merged the main-branch into my branch. How do I run the checks again?
--
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] cpoerschke commented on pull request #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1535948161
From a very quick look only, maybe `ModifiableSolrParams.java` could be a home for it?
--
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 #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1536057570
i like @cpoerschke suggestion, let's do that @renatoh and then we can 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.
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] renatoh commented on pull request #1570: Factor out ModifiableSolrParams.setShardAttributesToParams method
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1540914303
@epugh I have done 2 or 3 very small commits before, e.g. fixing a typo or doing a minor change to 1 or 2 lines of code.
Yes, that is OK, I would like to be referenced with my full name, which is Renato Haeberli.
Any idea why the job is failing again? I do not see any meaning full error message in the log
--
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] renatoh commented on pull request #1570: Factor out ModifiableSolrParams.setShardAttributesToParams method
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1551265172
@epugh are we able to merge that now? not sure if @risdenk will review it anytime soon
--
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] renatoh commented on pull request #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1538736352
@cpoerschke @epugh
As suggested I have move the method to ModifiableSolrParams.java
I had to change the method-signature slightly in order not to depend on org.apache.solr.handler.component.ShardRequest
--
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 #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1513018116
Good questions @renatoh.. I am not possiitve, however if you run `gradlew tidy` then it applies our spotless code formatter....
--
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 #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1511568532
@renatoh can you look at the precommit failure? I think a `tidy` is needed ;-)
--
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 #1570: Factor out ModifiableSolrParams.setShardAttributesToParams method
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1540938494
I don't know why, but yeah, Crave gets confused at times...
--
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 #1570: Factor out ModifiableSolrParams.setShardAttributesToParams method
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1540150494
@renatoh is this your first patch to solr? Normally small refactorings don't go into solr/CHANGES.txt, however when it's a new contributor, I like to mention them! Assuming that is okay, how owuld you like to be referenced?
--
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] renatoh commented on a diff in pull request #1570: Factor out ModifiableSolrParams.setShardAttributesToParams method
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on code in PR #1570:
URL: https://github.com/apache/solr/pull/1570#discussion_r1188697934
##########
solr/solrj/src/java/org/apache/solr/common/params/ModifiableSolrParams.java:
##########
@@ -175,6 +177,18 @@ public boolean remove(String name, String value) {
// ----------------------------------------------------------------
// ----------------------------------------------------------------
+ public static void setShardAttributesToParams(
+ int purpose, String shard, ModifiableSolrParams params) {
Review Comment:
true, I did not realize an instance of ModifiableSolrParams is present at the place where the method is called.
I have added a unit test validating the code, please have a look.
Also I have made two changes to the method, please have a look at me know if this makes sense to you.
Thanks!
--
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 #1570: Factor out ModifiableSolrParams.setShardAttributesToParams method
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #1570:
URL: https://github.com/apache/solr/pull/1570
--
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] renatoh commented on pull request #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1529042295
so in which class should the method be in?
--
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] cpoerschke commented on a diff in pull request #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #1570:
URL: https://github.com/apache/solr/pull/1570#discussion_r1188282152
##########
solr/solrj/src/java/org/apache/solr/common/params/ModifiableSolrParams.java:
##########
@@ -175,6 +177,18 @@ public boolean remove(String name, String value) {
// ----------------------------------------------------------------
// ----------------------------------------------------------------
+ public static void setShardAttributesToParams(
+ int purpose, String shard, ModifiableSolrParams params) {
Review Comment:
subjective: method could be non-static maybe and/or to add test coverage e.g. in `ModifiableSolrParamsTest.java`
```suggestion
public void setShardAttributes(
int purpose, String shard) {
```
--
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] renatoh commented on pull request #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1513014295
@epugh Thanks for the feedback. Is there an Intellij Code Style Scheme I can use in order to format the code correctly right within Intellij?
--
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] renatoh commented on pull request #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1513205591
@epugh so what tool is used within gradlew tidy to format the code? I am wondering it is possible to load these code style rules into a Intellij Code Style schema
I ran tidy and pushed the changes to my feature branch. is it possible to add the commit to the current PR or do I need to open a new PR?
--
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 #1570: Move duplicated code to method in TaskManagementHandler.java
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1570:
URL: https://github.com/apache/solr/pull/1570#issuecomment-1529020419
I *think* you pushed your tidy fixes up to the PR right? (Which is great!).. I clicked the "run precommit" github action...
--
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