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-turner (via GitHub)" <gi...@apache.org> on 2025/09/22 20:11:38 UTC
[PR] stops setting table prop in site config in ScanServerIT [accumulo]
keith-turner opened a new pull request, #5913:
URL: https://github.com/apache/accumulo/pull/5913
follow up to #5886
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2376436268
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
> I'm confused. Why were they hosted if hosting was never requested?
The test initially does [this](https://github.com/apache/accumulo/blob/77fd0df3e06d4dc4c9695ecd07b47f1fd18abb17/test/src/main/java/org/apache/accumulo/test/ScanServerIT.java#L369-L371) which causes all tablets in the table to be initially hosted. Then it eventually does [this](https://github.com/apache/accumulo/blob/77fd0df3e06d4dc4c9695ecd07b47f1fd18abb17/test/src/main/java/org/apache/accumulo/test/ScanServerIT.java#L383-L384) and the manager sees tablets that are ondemand, are hosted, and do not have the `requestToHost` to column set. When the manager sees this it computes a goal state of unassigned.
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2376657879
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
After the tablet is converted to ONDEMAND, should it be scanned again, to properly test the unload behavior based on the property? It seems that as the code evolved, the test stopped testing the case it was written for.
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2370196325
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
I was not sure which test methods needed this prop, so I removed it none failed. Have not looked into the history yet. Not sure if this in an indication of a bug w/ the test not meeting some expectation. @dlmarion do you know which test might expect 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2370220815
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
In the two tests that seem to be testing tablet availability, there is a utility method called `setupTableWithTabletAvailabilityMix`. The property can be added to the table at the time it is created in that method using NewTableConfiguration.
If the test is passing without it, though... it makes me wonder whether the test is actually doing what it is supposed to be doing. However, it's probably not a correctness thing, but an optimization thing, specifically for line 387 that is waiting 30 seconds for the inactivity-triggered unloads.
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2370205293
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
Probably related to the comment on line 261 about tablet unloading.
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #5913:
URL: https://github.com/apache/accumulo/pull/5913
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2376700803
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
> After the tablet is converted to ONDEMAND, should it be scanned again, to properly test the unload behavior based on the property?
This is a scan server test, so its not focused on testing that behavior it should just work w/ the expected behavior of that feature. There is OnDemandTabletUnloadingIT which does focus on testing that ondemand tablets that were hosted based on a hosting request do eventually unload.
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2370220815
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
In the two tests that same to be testing tablet availability, there is a utility method called `setupTableWithTabletAvailabilityMix`. The property can be added to the table at the time it is created in that method using NewTableConfiguration.
If the test is passing without it, though... it makes me wonder whether the test is actually doing what it is supposed to be doing. However, it's probably not a correctness thing, but an optimization thing, specifically for line 387 that is waiting 30 seconds for the inactivity-triggered unloads.
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2373095209
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
Looked into this and none of the test would benefit from setting this property. All of the test run faster than the `15s` that property was setting. Most test run in a few seconds except for one test that has a 30 second sleep and that is the what accounts for most of the time in that test.
Running the test I saw the following happen.
1. The tests creates 10 tablets with an initial tablet availability of hosted.
2. Then the test sets some of the tablets to ONDEMAND and the manager unloads these really quickly after that change.
The reason the manager unloads the tablets so quickly is because of [this code](https://github.com/apache/accumulo/blob/2c58d2924947d10ed3d018026885d5c24cc4fb87/server/base/src/main/java/org/apache/accumulo/server/manager/state/TabletGoalState.java#L105). The tablets were hosted w/o ever setting the hosting requested column, so when the manager sees an ondemand tablet w/o that column it computes a goal of unassigned. This is expected behavior.
So we do not need to set the property and setting it would be misleading.
Can also make the same change to fix ScanServer_NoServersIT and it also does not need to the property set for the same reasons.
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2376774586
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
Opened #5929
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2370232041
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
IIRC setting an operation id on the tablet will cause them to be unloaded as well. That could be why it's still passing.
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2376717187
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
This test is unintentionally testing the behavior of transitioning tablet availability from hosted to ondemand, would be good to add an intentional test for that to OnDemandTabletUnloadingIT in case this test case changes. I will look into doing 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2373663266
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
I'm confused. Why were they hosted if hosting was never requested?
If hosting was requested, what controls their inactivity time (i.e. what's the expected purpose of this property?)
It seems like the tests were trying to test tablet unload inactivity. If inactivity is not controlled through this property in any way that matters, are these tests even valid for what they were trying to test?
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2375450222
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
> Looked into this and none of the test would benefit from setting this property.
It may have made a difference earlier in the 4.0 development cycle. There have been changes since the initial ondemand tablet feature that made tablet unloading faster.
--
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] stops setting table prop in site config in ScanServerIT [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5913:
URL: https://github.com/apache/accumulo/pull/5913#discussion_r2376714221
##########
test/src/main/java/org/apache/accumulo/test/ScanServerIT.java:
##########
@@ -103,7 +103,6 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
cfg.setProperty(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL, "5");
cfg.setProperty(Property.TSERV_ONDEMAND_UNLOADER_INTERVAL, "10");
- cfg.setProperty("table.custom.ondemand.unloader.inactivity.threshold.seconds", "15");
Review Comment:
Okay, that makes sense.
--
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