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/07/04 10:46:55 UTC
[GitHub] [solr] bitnahian opened a new pull request #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
bitnahian opened a new pull request #202:
URL: https://github.com/apache/solr/pull/202
https://issues.apache.org/jira/browse/SOLR-15376
# Description
`CollectionAdminRequest.CreateTimeRoutedAlias.setMaxFutureMs` method only accepts an integer value as a parameter. The Java Integer.MAX_VALUE is `2147483647`. This value is too small to accommodate for any duration greater than ~ 25 days.
# Solution
Create an Overloaded method for setMaxFutureMs that accepts a Long type and change the type of instance variable `maxFutureMs` to a Long type and safely convert the original method's Integer type to a Long type in the original function.
# Tests
Attempted to run tests under org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java but tests are currently ignored.
# 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.
- [ ] 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] cpoerschke commented on pull request #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-897714718
Thanks @bitnahian for opening this pull request!
I'm not really familiar with this area of the code but via `git grep` noticed that https://github.com/apache/solr/blob/bd1ff339ab4c8d0d53bce9db66525b00c2f6da6f/solr/solrj/src/java/org/apache/solr/client/solrj/request/beans/CreateAliasPayload.java#L60 also has a integer `maxFutureMs` value currently. Not sure if that might benefit from changing to long type also.
cc: @gus-asf
--
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 #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-913644039
> Hmm, running the full test suite locally via `./gradlew test` gives two reproducible failures: ...
This is, I think, because the `@JsonProperty` annotations don't yet support `Long` type, separate #285 pull request and JIRA issue opened for that.
--
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 change in pull request #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #202:
URL: https://github.com/apache/solr/pull/202#discussion_r701247904
##########
File path: solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java
##########
@@ -713,6 +716,45 @@ public void testParse() {
TimeRoutedAlias.parseInstantFromCollectionName(alias, alias + TRA + "2017-10-02"));
}
+ @Test
+ public void testMaxFutureMs() throws Exception {
+ final Long maxFutureMs = 100L * 24L * 60L * 60L * 1000L; // 100 DAYS
+
+ ZonedDateTime a = ZonedDateTime.now(ZoneId.from(ZoneOffset.UTC));
+ ZonedDateTime b = a.plusMonths(2);
+ ZonedDateTime c = a.plusYears(1);
+ DateTimeFormatter SOLR_DATE_TIME_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss'Z'").withZone(ZoneId.from(ZoneOffset.UTC));
Review comment:
```suggestion
DateTimeFormatter SOLR_DATE_TIME_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss'Z'", Locale.ROOT).withZone(ZoneId.from(ZoneOffset.UTC));
```
or similar is what the CI feedback is probably saying; `./gradlew :solr:core:forbiddenApisTest` to run it locally.
--
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 #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-929215631
> So I'll just wait on the other issue to get resolved then? Any TO-DOs before then?
No TO-DOs that I can think of, thanks for the ping! Once the other issue is resolved the latest `solr/main` branch will need merging into this pull request's branch here.
--
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 #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-898673372
> Attempted to run tests under org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java but tests are currently ignored.
Looks like `-Ptests.filter=@AwaitsFix` needs to be added to run the tests e.g.
```
./gradlew test --tests org.apache.solr.update.processor.TimeRoutedAliasUpdateProcessorTest.testMaxFutureMs -Ptests.filter=@AwaitsFix
```
for the test I've added in the commit above.
What do you think? Feel free to revert or amend, I've left some TODO comments on how it might change to use the new 'Long' type functionality.
--
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 merged pull request #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
cpoerschke merged pull request #202:
URL: https://github.com/apache/solr/pull/202
--
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] bitnahian edited a comment on pull request #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
bitnahian edited a comment on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-912266646
@cpoerschke Nahian-Al Hasan is fine :D
--
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 #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-929215631
> So I'll just wait on the other issue to get resolved then? Any TO-DOs before then?
No TO-DOs that I can think of, thanks for the ping! Once the other issue is resolved the latest `solr/main` branch will need merging into this pull request's branch here.
--
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] bitnahian commented on pull request #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
bitnahian commented on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-911154201
Hi @cpoerschke, I've completed the TO-DOs suggested in your commit. :)
--
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] bitnahian commented on pull request #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
bitnahian commented on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-912266646
@cpoerschke bitnahian is fine :D
--
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 #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-912684782
Hmm, running the full test suite locally via `./gradlew test` gives two reproducible failures:
```
gradlew :solr:core:test --tests "org.apache.solr.handler.admin.V2CollectionsAPIMappingTest.testCreateAliasAllProperties" -Ptests.jvms=4 -Ptests.jvmargs=-XX:TieredStopAtLevel=1 -Ptests.seed=D0AC04911DD2F9B9 -Ptests.file.encoding=US-ASCII
gradlew :solr:core:test --tests "org.apache.solr.cloud.CreateRoutedAliasTest.testV2" -Ptests.jvms=4 -Ptests.jvmargs=-XX:TieredStopAtLevel=1 -Ptests.seed=D0AC04911DD2F9B9 -Ptests.file.encoding=US-ASCII
```
--
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] bitnahian commented on pull request #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
bitnahian commented on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-898142889
@cpoerschke Thanks for the feedback. Good catch. Missed 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] cpoerschke commented on pull request #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-911854652
Thanks @bitnahian for completing the test TO-DO items!
I just added two commits to the branch:
* @gus-asf's suggestion to deprecate the Integer method variant
* a draft solr/CHANGES.txt entry, question: would you prefer your name (Nahian-Al Hasan) or your github handle (bitnahian) or something else in the attribution?
--
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] bitnahian commented on pull request #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
bitnahian commented on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-928638789
So I'll just wait on the other issue to get resolved then? Any TO-DOs before then?
--
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] bitnahian commented on pull request #202: SOLR-15376 Adding setMaxFutureMs with Long type Overload method
Posted by GitBox <gi...@apache.org>.
bitnahian commented on pull request #202:
URL: https://github.com/apache/solr/pull/202#issuecomment-928638789
So I'll just wait on the other issue to get resolved then? Any TO-DOs before then?
--
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