You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-ratcliffe (via GitHub)" <gi...@apache.org> on 2023/07/18 23:12:38 UTC
[GitHub] [accumulo] keith-ratcliffe opened a new pull request, #3631: Fixes #3630 - ClientContext updated to handle time duration values correctly
keith-ratcliffe opened a new pull request, #3631:
URL: https://github.com/apache/accumulo/pull/3631
(no comment)
--
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 a diff in pull request #3631: Fixes #3630 - ClientContext updated to handle time duration values correctly
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3631:
URL: https://github.com/apache/accumulo/pull/3631#discussion_r1267907393
##########
core/src/test/java/org/apache/accumulo/core/clientImpl/ClientContextTest.java:
##########
@@ -159,10 +159,9 @@ public void testGetBatchWriterConfigNotUsingDefaults() {
.getMemoryAsBytes(ClientProperty.BATCH_WRITER_MEMORY_MAX.getValue(props));
assertEquals(expectedMemory, batchWriterConfig.getMaxMemory());
- assertEquals(Long.MAX_VALUE, batchWriterConfig.getMaxLatency(MILLISECONDS));
+ assertEquals(40, batchWriterConfig.getMaxLatency(SECONDS));
Review Comment:
Shouldn't the units here be milliseconds?
##########
core/src/test/java/org/apache/accumulo/core/clientImpl/ClientContextTest.java:
##########
@@ -159,10 +159,9 @@ public void testGetBatchWriterConfigNotUsingDefaults() {
.getMemoryAsBytes(ClientProperty.BATCH_WRITER_MEMORY_MAX.getValue(props));
assertEquals(expectedMemory, batchWriterConfig.getMaxMemory());
- assertEquals(Long.MAX_VALUE, batchWriterConfig.getMaxLatency(MILLISECONDS));
+ assertEquals(40, batchWriterConfig.getMaxLatency(SECONDS));
- // getTimeout returns time in milliseconds, therefore the 15 becomes 15000.
- assertEquals(15000, batchWriterConfig.getTimeout(SECONDS));
+ assertEquals(15, batchWriterConfig.getTimeout(SECONDS));
Review Comment:
Shouldn't the units here be milliseconds?
--
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 merged pull request #3631: Fixes #3630 - ClientContext updated to handle time duration values correctly
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #3631:
URL: https://github.com/apache/accumulo/pull/3631
--
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 #3631: Fixes #3630 - ClientContext updated to handle time duration values correctly
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3631:
URL: https://github.com/apache/accumulo/pull/3631#issuecomment-1643793485
@keith-ratcliffe - Thanks for your contribution. I see this is your first time contributing. Feel free to put up a PR on https://github.com/apache/accumulo-website to add yourself to the people page.
--
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-ratcliffe commented on a diff in pull request #3631: Fixes #3630 - ClientContext updated to handle time duration values correctly
Posted by "keith-ratcliffe (via GitHub)" <gi...@apache.org>.
keith-ratcliffe commented on code in PR #3631:
URL: https://github.com/apache/accumulo/pull/3631#discussion_r1268224736
##########
core/src/test/java/org/apache/accumulo/core/clientImpl/ClientContextTest.java:
##########
@@ -159,10 +159,9 @@ public void testGetBatchWriterConfigNotUsingDefaults() {
.getMemoryAsBytes(ClientProperty.BATCH_WRITER_MEMORY_MAX.getValue(props));
assertEquals(expectedMemory, batchWriterConfig.getMaxMemory());
- assertEquals(Long.MAX_VALUE, batchWriterConfig.getMaxLatency(MILLISECONDS));
+ assertEquals(40, batchWriterConfig.getMaxLatency(SECONDS));
- // getTimeout returns time in milliseconds, therefore the 15 becomes 15000.
- assertEquals(15000, batchWriterConfig.getTimeout(SECONDS));
+ assertEquals(15, batchWriterConfig.getTimeout(SECONDS));
Review Comment:
Same reply as above
--
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-ratcliffe commented on pull request #3631: Fixes #3630 - ClientContext updated to handle time duration values correctly
Posted by "keith-ratcliffe (via GitHub)" <gi...@apache.org>.
keith-ratcliffe commented on PR #3631:
URL: https://github.com/apache/accumulo/pull/3631#issuecomment-1642346440
Same bug [here](https://github.com/apache/accumulo/blob/2.1/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java#L436) which I missed, for conditional writer timeout. The test uses an prop value of "17" (defaulting to time unit of SECONDS) and then [asserts that 17000 seconds is expected](https://github.com/apache/accumulo/blob/2.1/core/src/test/java/org/apache/accumulo/core/clientImpl/ClientContextTest.java#L219)
--
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 a diff in pull request #3631: Fixes #3630 - ClientContext updated to handle time duration values correctly
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3631:
URL: https://github.com/apache/accumulo/pull/3631#discussion_r1268481224
##########
core/src/test/java/org/apache/accumulo/core/clientImpl/ClientContextTest.java:
##########
@@ -159,10 +159,9 @@ public void testGetBatchWriterConfigNotUsingDefaults() {
.getMemoryAsBytes(ClientProperty.BATCH_WRITER_MEMORY_MAX.getValue(props));
assertEquals(expectedMemory, batchWriterConfig.getMaxMemory());
- assertEquals(Long.MAX_VALUE, batchWriterConfig.getMaxLatency(MILLISECONDS));
+ assertEquals(40, batchWriterConfig.getMaxLatency(SECONDS));
Review Comment:
I see, the default without units is SECONDS.
--
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-ratcliffe commented on a diff in pull request #3631: Fixes #3630 - ClientContext updated to handle time duration values correctly
Posted by "keith-ratcliffe (via GitHub)" <gi...@apache.org>.
keith-ratcliffe commented on code in PR #3631:
URL: https://github.com/apache/accumulo/pull/3631#discussion_r1268210983
##########
core/src/test/java/org/apache/accumulo/core/clientImpl/ClientContextTest.java:
##########
@@ -159,10 +159,9 @@ public void testGetBatchWriterConfigNotUsingDefaults() {
.getMemoryAsBytes(ClientProperty.BATCH_WRITER_MEMORY_MAX.getValue(props));
assertEquals(expectedMemory, batchWriterConfig.getMaxMemory());
- assertEquals(Long.MAX_VALUE, batchWriterConfig.getMaxLatency(MILLISECONDS));
+ assertEquals(40, batchWriterConfig.getMaxLatency(SECONDS));
Review Comment:
Sorry, I may be misunderstanding the question. The TimeUnit arg to getMaxLatency could be changed to MILLISECONDS, assuming a corresponding change in _expected_ arg from 40 to 40000. But that wouldn't change the validity of the assertion, right?
--
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-ratcliffe commented on pull request #3631: Fixes #3630 - ClientContext updated to handle time duration values correctly
Posted by "keith-ratcliffe (via GitHub)" <gi...@apache.org>.
keith-ratcliffe commented on PR #3631:
URL: https://github.com/apache/accumulo/pull/3631#issuecomment-1642513840
Rebased/squash commit now includes the fix for CONDITIONAL_WRITER_TIMEOUT_MAX and also cleanup/refinement of the affected junits in ClientContextTest
--
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