You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2024/04/12 14:29:18 UTC
[PR] Ignore TABLE_SCAN_MAXMEM for root table [accumulo]
dlmarion opened a new pull request, #4453:
URL: https://github.com/apache/accumulo/pull/4453
(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
Re: [PR] Ignore TABLE_SCAN_MAXMEM for root table [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion closed pull request #4453: Ignore TABLE_SCAN_MAXMEM for root table
URL: https://github.com/apache/accumulo/pull/4453
--
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
Re: [PR] Ignore TABLE_SCAN_MAXMEM for root table [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4453:
URL: https://github.com/apache/accumulo/pull/4453#issuecomment-2052254364
>We could also improve the logging around the memory check to make the issue more obvious but just not letting it easily happen in the first place makes sense to me.
> But if a user specifically tries to set a lower value on a system table like metadata then maybe we should throw an exception and prevent that config change as it would just be ignored anyways.
I'm wondering if instead of changing the code to throw an exception, we just document the behavior change in Property.java. Something like `Values lower than the default value are ignored for system tables.`
--
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
Re: [PR] Ignore TABLE_SCAN_MAXMEM for root table [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4453:
URL: https://github.com/apache/accumulo/pull/4453#discussion_r1562654000
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/scan/LookupTask.java:
##########
@@ -76,6 +77,11 @@ public void run() {
TableConfiguration acuTableConf = server.getTableConfiguration(session.threadPoolExtent);
long maxResultsSize = acuTableConf.getAsBytes(Property.TABLE_SCAN_MAXMEM);
+ // Always return large root tablets
+ if (session.threadPoolExtent.tableId().equals(AccumuloTable.ROOT.tableId())) {
+ maxResultsSize = Long.MAX_VALUE;
Review Comment:
I agree that going smaller than Long.MAX_VALUE makes sense.
Maybe the best thing is to just take the maximum value between the default and the user setting so we always use at least the default size but allow users to go higher.
```java
maxResultsSize = Math.max(maxResultsSize, ConfigurationTypeHelper
getFixedMemoryAsBytes(Property.TABLE_SCAN_MAXMEM.getDefaultValue()));
```
--
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
Re: [PR] Ignore TABLE_SCAN_MAXMEM for root table [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4453:
URL: https://github.com/apache/accumulo/pull/4453#issuecomment-2051872396
I'm curious if we should ignore this property for all tables in the accumulo namespace for proper function of the system.
--
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
Re: [PR] Ignore TABLE_SCAN_MAXMEM for root table [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #4453:
URL: https://github.com/apache/accumulo/pull/4453#issuecomment-2052264009
> I'm wondering if instead of changing the code to throw an exception, we just document the behavior change in Property.java. Something like `Values lower than the default value are ignored for system tables.`
That's probably fine too if it is documented so that users are at least aware it will be ignored and it isn't unexpected.
--
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
Re: [PR] Ignore TABLE_SCAN_MAXMEM for root table [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4453:
URL: https://github.com/apache/accumulo/pull/4453#discussion_r1562637889
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/scan/LookupTask.java:
##########
@@ -76,6 +77,11 @@ public void run() {
TableConfiguration acuTableConf = server.getTableConfiguration(session.threadPoolExtent);
long maxResultsSize = acuTableConf.getAsBytes(Property.TABLE_SCAN_MAXMEM);
+ // Always return large root tablets
+ if (session.threadPoolExtent.tableId().equals(AccumuloTable.ROOT.tableId())) {
+ maxResultsSize = Long.MAX_VALUE;
Review Comment:
The property default is 512K, so maybe Long.MAX_VALUE is not the right choice 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ignore TABLE_SCAN_MAXMEM for root table [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4453:
URL: https://github.com/apache/accumulo/pull/4453#issuecomment-2056634311
Closing as #4457 fixes the root cause.
--
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
Re: [PR] Ignore TABLE_SCAN_MAXMEM for root table [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4453:
URL: https://github.com/apache/accumulo/pull/4453#discussion_r1562990341
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/scan/LookupTask.java:
##########
@@ -76,6 +77,11 @@ public void run() {
TableConfiguration acuTableConf = server.getTableConfiguration(session.threadPoolExtent);
long maxResultsSize = acuTableConf.getAsBytes(Property.TABLE_SCAN_MAXMEM);
+ // Always return large root tablets
+ if (session.threadPoolExtent.tableId().equals(AccumuloTable.ROOT.tableId())) {
+ maxResultsSize = Long.MAX_VALUE;
Review Comment:
Implemented in 13249fb
--
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