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 2024/12/17 23:08:55 UTC
[PR] Fixes balance related filtering that was using table name instead of id [accumulo]
keith-turner opened a new pull request, #5195:
URL: https://github.com/apache/accumulo/pull/5195
(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] Fixes balancing of metadata table [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #5195:
URL: https://github.com/apache/accumulo/pull/5195#issuecomment-2682415872
Applied the BalanceIT test changes to current 2.1 and replicated the breakage.
Then stepped through applying commits individually to understand them.
> 1. Metadata table was not balancing
Confirmed this is fixed by 6eab50f5e0d34394cb48a82314edf56fcbb130b1
> 3. Each call to the balancer would balance all tables. For example when balancing the root table it would compute balancing for all tables.
This could be fixed in the current code by updating the balance method in TableLoadBalancer.
https://github.com/apache/accumulo/blob/aa7944b621ffc669917e2a30cd4c163bee760b48/core/src/main/java/org/apache/accumulo/core/spi/balancer/TableLoadBalancer.java#L125-L133
It just needs to check the datalevel level of the tableId and just skipping that tableID if it does not match the current level.
```
if (DataLevel.of(tableId) != currentDataLevel) {
// Don't balance tables that are outside the current data level.
log.info("Skipping Table {} as it is not in the current data level", tableId);
continue;
}
```
> 2. Sometimes user tables would balance when the metadata table was not balanced
I like the new migrations class but I'd like to understand how it fits into the overall balancing design.
We've made a good deal of changes trying to fix balancing bugs in 2.1 and it might be a good time to take a step back and re-examine it.
--
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] Fixes balancing of metadata table [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #5195:
URL: https://github.com/apache/accumulo/pull/5195#issuecomment-2644028795
Adding a blocker label as this change is needed before finalizing the 2.1.4 release.
--
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] Fixes balance related filtering that was using table name instead of id [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5195:
URL: https://github.com/apache/accumulo/pull/5195#issuecomment-2552088430
Working on adding an IT for metadata table balancing.
--
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] Fixes balance related filtering that was using table name instead of id [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5195:
URL: https://github.com/apache/accumulo/pull/5195#discussion_r1890706290
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1000,20 +1000,22 @@ private SortedMap<TServerInstance,TabletServerStatus> createTServerStatusView(
final Map<String,TableInfo> newTableMap =
new HashMap<>(dl == DataLevel.USER ? oldTableMap.size() : 1);
if (dl == DataLevel.ROOT) {
- if (oldTableMap.containsKey(RootTable.NAME)) {
- newTableMap.put(RootTable.NAME, oldTableMap.get(RootTable.NAME));
+ if (oldTableMap.containsKey(RootTable.ID.canonical())) {
Review Comment:
> Why not make it a Map<TableId,TableInfo> instead of Map<String,TableInfo>?
The code is currently manipulating a Thrift object which can not use the TableId type. A larger refactor would be needed to make that change, could open in issue to look into 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] Fixes balancing of metadata table [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5195:
URL: https://github.com/apache/accumulo/pull/5195#discussion_r1970706543
##########
core/src/main/java/org/apache/accumulo/core/spi/balancer/GroupBalancer.java:
##########
@@ -213,7 +213,7 @@ public long balance(BalanceParameters params) {
return 5000;
}
- final DataLevel currentLevel = DataLevel.valueOf(params.currentLevel());
+ final DataLevel currentLevel = DataLevel.valueOf(params.partitionName());
Review Comment:
I think this comment is against an older commit, but not sure. When I look at that line in the latest code do not see 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] Fixes balance related filtering that was using table name instead of id [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5195:
URL: https://github.com/apache/accumulo/pull/5195#issuecomment-2557914996
After writing the new IT I saw the following problems, with the first being the initial reason I opened this PR.
1. Metadata table was not balancing
2. Sometimes user tables would balance when the metadata table was not balanced
3. Each call to the balancer would balance all tables. For example when balancing the root table it would compute balancing for all tables.
For 1 the changes in 6eab50f fix this. For 2 and 3 the changes in 6de5ef9 fix these. For 2 created a new Migrations class that tracks migrations per data level so that when making balancing decisions it could always know if the previous levels had migrations. For 3 added a new method to the balancer SPI that conveys what tables need to be balanced. This new method support partitioning tables for balancing in different ways and allows balancers to delegate to other balancers with smaller partitions. May also allow different manager processes to parition tables for balancing in the future.
Took this out of draft after the changes in 6de5ef9
--
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] Fixes balancing of metadata table [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #5195:
URL: https://github.com/apache/accumulo/pull/5195#issuecomment-2688957457
All changes from this PR should have been addressed in #5358, #5359, and #5363.
--
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] Fixes balancing of metadata table [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5195:
URL: https://github.com/apache/accumulo/pull/5195#issuecomment-2683575416
> It just needs to check the datalevel level of the tableId and just skipping that tableID if it does not match the current level.
Sometimes a balancer is expected to work with a smaller set of tables than the entire data level (like when the tablet balancer create a sub balancer). This is why I added the set of tables to balance as a map. This make it less mysterious, instead of using an arbitrary string to somehow determine which subset of tables to balance its given in a set.
--
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] Fixes balancing of metadata table [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner closed pull request #5195: Fixes balancing of metadata table
URL: https://github.com/apache/accumulo/pull/5195
--
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] Fixes balance related filtering that was using table name instead of id [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5195:
URL: https://github.com/apache/accumulo/pull/5195#issuecomment-2552452665
Wrote a test and found some additional issues and made fixes to get the test to pass. Converted this PR to draft because I want to go review the changes I made to get the test to pass.
--
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] Fixes balance related filtering that was using table name instead of id [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5195:
URL: https://github.com/apache/accumulo/pull/5195#issuecomment-2549849296
Have not tested these changes. Made similar changes in 4.0 and manually tested them. We probably need an IT that test balancing the metadata table if there is not one. In 4.0 this issue was preventing the metadata table from balancing across multiple tablet servers.
--
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] Fixes balancing of metadata table [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #5195:
URL: https://github.com/apache/accumulo/pull/5195#discussion_r1970550544
##########
core/src/main/java/org/apache/accumulo/core/spi/balancer/GroupBalancer.java:
##########
@@ -213,7 +213,7 @@ public long balance(BalanceParameters params) {
return 5000;
}
- final DataLevel currentLevel = DataLevel.valueOf(params.currentLevel());
+ final DataLevel currentLevel = DataLevel.valueOf(params.partitionName());
Review Comment:
since `valueOf` is an enum method, won't this throw an `IllegalArgumentException` if params has a custom partitionName?
--
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] Fixes balance related filtering that was using table name instead of id [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5195:
URL: https://github.com/apache/accumulo/pull/5195#discussion_r1889452799
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1000,20 +1000,22 @@ private SortedMap<TServerInstance,TabletServerStatus> createTServerStatusView(
final Map<String,TableInfo> newTableMap =
new HashMap<>(dl == DataLevel.USER ? oldTableMap.size() : 1);
if (dl == DataLevel.ROOT) {
- if (oldTableMap.containsKey(RootTable.NAME)) {
- newTableMap.put(RootTable.NAME, oldTableMap.get(RootTable.NAME));
+ if (oldTableMap.containsKey(RootTable.ID.canonical())) {
Review Comment:
Why not make it a `Map<TableId,TableInfo>` instead of `Map<String,TableInfo>`?
--
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