You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2023/01/20 16:51:39 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #3166: rename ScannerOptions timeOut to scanTimeOut, fix possible bug
EdColeman opened a new pull request, #3166:
URL: https://github.com/apache/accumulo/pull/3166
- rename variable in ScannerOptions from timeOut to scanTimeOut
- fixes possible bug where input variable (timeout) was using the instance value for checks
- rename variable in IsolatedScanner from timeOut to iteratorTimeout
- fix some flagged grammar suggestions.
In IsolatedScanner, the two timeouts are confusing and it could be an error that there are two values when one was intended, but I preserved current behavior.
There are other code style issues that can be addressed in these files, but I didn't want those changes to hide the timeout renaming with even more unrelated changes.
I ran into the timeout naming while trying to look at synchronization warnings and having timeout, and timeOut available as valid code completion options made it harder to reason about the correct value.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3166: rename ScannerOptions timeOut to scanTimeOut, fix possible bug
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3166:
URL: https://github.com/apache/accumulo/pull/3166#issuecomment-1399573166
> In IsolatedScanner, the two timeouts are confusing and it could be an error that there are two values when one was intended, but I preserved current behavior.
@EdColeman can you provide a pointer to the code that is suspect?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3166: rename ScannerOptions timeOut to scanTimeOut, fix possible bug
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3166:
URL: https://github.com/apache/accumulo/pull/3166#issuecomment-1398817770
It does not seem to resolve #2606, I ran it with this change and the test still fails. @DomGarguilo noted that the slow iterator is pausing between calls to next which may be different that the time of the scan itself.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3166: rename ScannerOptions timeOut to scanTimeOut, fix possible bug
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3166:
URL: https://github.com/apache/accumulo/pull/3166#issuecomment-1402838794
Closing this and will submit a new PR against 2.1. After more investigation timeOut should be renamed retryTimeout and there may be other changes that can provide readability improvements.
The testing of the scan timeout here is invalid and is not testing the expected conditions.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3166: rename ScannerOptions timeOut to scanTimeOut, fix possible bug
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3166:
URL: https://github.com/apache/accumulo/pull/3166#issuecomment-1400834950
As @DomGarguilo noted in https://github.com/apache/accumulo/issues/2606 - I'm not sure that the timeouts being tested are actually the scanner timeouts. There seems to be a sensitivity to the timeout value used that is independent of the scan timeout that is set. SO, I'm not sure, in there current for the added tests are testing what was intended.
Some possibilities:
- The SlowIterator used does not trigger a timeout that would be guarded by the scan timeout.
- There is an issue with the timeout in the scanner.
- something else?
Still looking at it and resolution may be independent of this 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3166: rename ScannerOptions timeOut to scanTimeOut, fix possible bug
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3166:
URL: https://github.com/apache/accumulo/pull/3166#issuecomment-1398819178
ok, thx for checking.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3166: rename ScannerOptions timeOut to scanTimeOut, fix possible bug
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3166:
URL: https://github.com/apache/accumulo/pull/3166#issuecomment-1400652298
@keith-turner Basically there are two timeouts in IsolatedScanner - one is inherited by extending ScannerOptions and the other is local to IsolatedScanner (now named iteratorTimeout in this PR) and referenced below as the "local" value. The scannerTimeout in Scanner options is not referenced in the IsolatedScanner code. I was unsure if the local one was necessary or if the code could delegate to ScannerOptions.
Before, the local IsolatedScanner timeout was "timeout"
https://github.com/apache/accumulo/blob/459a5c0d5ef97339837a75b61af817f77fc65b42/core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java#L52
In ScannerOptions, the parameter was called "timeOut" (note capital O)
https://github.com/apache/accumulo/blob/459a5c0d5ef97339837a75b61af817f77fc65b42/core/src/main/java/org/apache/accumulo/core/clientImpl/ScannerOptions.java#L56
When the iterator is created in newIterator, the scanner is set to the "locally declared value"
https://github.com/apache/accumulo/blob/0ef06afbfcea0908d3b19e1d8ae9fc1cc60a2e77/core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java#L124
In the buffered reader the constructor, it sets the "local" value.
https://github.com/apache/accumulo/blob/0ef06afbfcea0908d3b19e1d8ae9fc1cc60a2e77/core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java#L137
I was unsure if "timeout" was distinctly different than "timeOut" or if it was an oversight. I thought about deleting the local value and delegating to the scannerTimeout (timeOut) in ScannerOptions, but it was not clear if this would be okay or if the local value was specifically created / used for some purpose that was not clear to me.
It's fine if the local value is needed / stays - but a short description as to why may help the next person that comes along and has the same question. The other option is to delegate to the ScannerOptions scannerTimeout (timeOut) and remove the "local" value.
Currently, this PR is keeping both to preserve the current 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3166: rename ScannerOptions timeOut to scanTimeOut, fix possible bug
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3166:
URL: https://github.com/apache/accumulo/pull/3166#issuecomment-1398803887
Could this be the cause of #2606 ?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman closed pull request #3166: rename ScannerOptions timeOut to scanTimeOut, fix possible bug
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman closed pull request #3166: rename ScannerOptions timeOut to scanTimeOut, fix possible bug
URL: https://github.com/apache/accumulo/pull/3166
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3166: rename ScannerOptions timeOut to scanTimeOut, fix possible bug
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3166:
URL: https://github.com/apache/accumulo/pull/3166#issuecomment-1398893342
30c674cbd8 modifies the ScanServerIT to test scan timeout - may resolve #2606
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3166: rename ScannerOptions timeOut to scanTimeOut, fix possible bug
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3166:
URL: https://github.com/apache/accumulo/pull/3166#issuecomment-1404054373
Superseded by #3172
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org