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 2023/11/03 17:47:04 UTC
[PR] Replaced last location mode of 'compaction' with 'none' [accumulo]
dlmarion opened a new pull request, #3926:
URL: https://github.com/apache/accumulo/pull/3926
Replaced last location mode of 'compaction' with 'none' which allows the last location to be unset when either a hosted tablet is unloaded or for an unhosted tablet when the TabletGroupWatcher processes its metadata.
Fixes #3596
--
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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #3926:
URL: https://github.com/apache/accumulo/pull/3926
--
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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#issuecomment-1809180852
> Wondering it is would be better to remove the `tserver.last.location.mode` property and always set the last location on assignment. There would be less code to maintain and test (do not need to test the property or handle removing last when its set to none). For the case where a balancer plugin is not interested in the last location information, it can ignore it.
Can we emit information, such as last assignment, last hosted location for write, etc. that users can use to empower their own pluggable balancer? It seems like historically we made assumptions about how users might want to balance, and then we had to add this property to handle some additional use cases that users preferred, but it'd probably better to just to emit data that the users might want to use, and leave the balancing strategy up to them. That would maximally empower users and minimize the code we need to maintain (except for the code to emit the requested info, which should be simpler, and generally useful enough to warrant any complexity it might require).
--
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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#issuecomment-1815156943
> > I think the none value supports the following use-cases:
>
> @dlmarion for those use cases I was thinking another way to achieve them would be to configure the balancer to ignore the last location.
So, I understand your approach here, but I think I'll need you to elaborate on your idea from an implementation perspective. I assume that this is a new property that the user can set on a table, and that we need to modify all of the balancers to look for this property?
--
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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#issuecomment-1819149683
Follow-on tasks that need to be created:
1. in main, deprecate TSERV_LAST_LOCATION_MODE
2. in elasticity, modify upgrader to error if 'tserver.last.location.mode' is set (maybe make it more general such that if any property is set that does not exist, then fail to upgrade)
--
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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#issuecomment-1813153951
> I think the none value supports the following use-cases:
@dlmarion for those use cases I was thinking another way to achieve them would be to configure the balancer to ignore the last location.
--
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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#issuecomment-1808625257
Wondering it is would be better to remove the `tserver.last.location.mode` property and always set the last location on assignment. There would be less code to maintain and test (do not need to test the property or handle removing last when its set to none). For the case where a balancer plugin is not interested in the last location information, it can ignore 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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#issuecomment-1810239500
> @keith-turner said, "Wondering it is would be better to remove the tserver.last.location.mode property and always set the last location on assignment."
I think the `none` value supports the following use-cases:
1. Deployment to an orchestrated environment (e.g. Kubernetes). I'm not sure how meaningful, or repeatable, hostnames are in this environment. Using `none` here allows the tablet to be hosted on one of the TabletServers in the assigned resource group.
2. When an admin wants to unset the last location for a tables tablets allowing the configured balancer to make a new initial determination.
> @ctubbsii said, "Can we emit information..."
When you say emit, I think metrics. Is that what you mean?
--
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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#discussion_r1399917467
##########
server/base/src/main/java/org/apache/accumulo/server/util/ManagerMetadataUtil.java:
##########
@@ -177,45 +175,11 @@ private static KeyExtent fixSplit(ServerContext context, TableId tableId, Text m
}
}
- /**
- * Update the last location if the location mode is "assignment". This will delete the previous
- * last location if needed and set the new last location
- *
- * @param context The server context
- * @param tabletMutator The mutator being built
- * @param location The new location
- * @param lastLocation The previous last location, which may be null
- */
- public static void updateLastForAssignmentMode(ClientContext context,
- Ample.TabletUpdates<?> tabletMutator, TServerInstance location, Location lastLocation) {
+ public static void updateLastLocation(Ample.TabletUpdates<?> tabletMutator,
Review Comment:
This ManagerMetadataUtil code is only used by old tablet server split recovery code that needs to be migrated to upgrade code eventually. It would be nice to move this static method to AbstractTabletStateStore as that is the only thing that uses (except for unit test which could be renamed).
Could also inline updateLocation into this method as this method is the only thing that calls it.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1645,7 +1645,10 @@ void getAssignments(SortedMap<TServerInstance,TabletServerStatus> currentStatus,
AssignmentParamsImpl params =
AssignmentParamsImpl.fromThrift(currentStatus, currentTServerGroups,
unassigned.entrySet().stream().collect(HashMap::new,
- (m, e) -> m.put(e.getKey(), e.getValue().getServerInstance()), Map::putAll),
Review Comment:
Was this an existing NPE bug?
--
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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#issuecomment-1821642287
> Follow-on tasks that need to be created:
>
> 1. in main, deprecate TSERV_LAST_LOCATION_MODE
> 2. in elasticity, modify upgrader to error if 'tserver.last.location.mode' is set (maybe make it more general such that if any property is set that does not exist, then fail to upgrade)
Follow-on tasks created.
--
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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#issuecomment-1819063879
> @dlmarion asked:
>
> > > @ctubbsii said, "Can we emit information..."
> >
> >
> > When you say emit, I think metrics. Is that what you mean?
>
> Metrics, logging, some pluggable interface to collect state, it doesn't really matter to me. I was mostly describing the general idea of shifting away from focusing on providing a specific end-to-end solution to providing the bits for users to create their own end-to-end solution. So, instead of us tracking the last write/compaction, or the last assignment, in our metadata and providing specific config knobs for that behavior, and then wiring that up for users as an option to a pre-provisioned balancer, we should maybe focus our implementations on providing insights (metrics is one way) to users, and pluggable interfaces for users to make their own choices about how to use those insights. This is just a better overall strategy, because it gives users more flexibility to support a variety of use cases we haven't thought of in advance, rather than constraining user imagination by locking them in on our specific thoughts. It also has the potential to dramatically reduce baked-in co
mplexity and code size that we need to maintain.
>
> How this idea comes into play here: don't bother changing the property options and behavior... just deprecate it and create an emitter (metrics, or whatever) that users can collect information from, that records things like details on assignment activity or details on compaction activity. Then, leave it to users to wire that information in to their own custom balancer, or wherever else they want to use it.
The last known location is provided to the balancer SPI in [AssignmentParameters.unassignedTablets](https://github.com/apache/accumulo/blob/main/core/src/main/java/org/apache/accumulo/core/spi/balancer/TabletBalancer.java#L61). I think for now I'll just remove the property and the update last assignment in TabletMetadata.
--
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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#issuecomment-1816658764
> So, I understand your approach here, but I think I'll need you to elaborate on your idea from an implementation perspective. I assume that this is a new property that the user can set on a table, and that we need to modify all of the balancers to look for this property?
Yeah I was thinking this could be per balancer plugin configuration, not something that is baked into Accumulo. A plugin implementation could have configuration that controls what it will do with the last location information. Or a plugin could have no configuration options for 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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#issuecomment-1816611392
@dlmarion asked:
> > @ctubbsii said, "Can we emit information..."
>
> When you say emit, I think metrics. Is that what you mean?
Metrics, logging, some pluggable interface to collect state, it doesn't really matter to me. I was mostly describing the general idea of shifting away from focusing on providing a specific end-to-end solution to providing the bits for users to create their own end-to-end solution. So, instead of us tracking the last write/compaction, or the last assignment, in our metadata and providing specific config knobs for that behavior, and then wiring that up for users as an option to a pre-provisioned balancer, we should maybe focus our implementations on providing insights (metrics is one way) to users, and pluggable interfaces for users to make their own choices about how to use those insights. This is just a better overall strategy, because it gives users more flexibility to support a variety of use cases we haven't thought of in advance, rather than constraining user imagination by locking them in on our specific thoughts. It also has the potential to dramatically reduce baked-in comp
lexity and code size that we need to maintain.
How this idea comes into play here: don't bother changing the property options and behavior... just deprecate it and create an emitter (metrics, or whatever) that users can collect information from, that records things like details on assignment activity or details on compaction activity. Then, leave it to users to wire that information in to their own custom balancer, or wherever else they want to use 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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#discussion_r1401122231
##########
server/base/src/main/java/org/apache/accumulo/server/util/ManagerMetadataUtil.java:
##########
@@ -177,45 +175,11 @@ private static KeyExtent fixSplit(ServerContext context, TableId tableId, Text m
}
}
- /**
- * Update the last location if the location mode is "assignment". This will delete the previous
- * last location if needed and set the new last location
- *
- * @param context The server context
- * @param tabletMutator The mutator being built
- * @param location The new location
- * @param lastLocation The previous last location, which may be null
- */
- public static void updateLastForAssignmentMode(ClientContext context,
- Ample.TabletUpdates<?> tabletMutator, TServerInstance location, Location lastLocation) {
+ public static void updateLastLocation(Ample.TabletUpdates<?> tabletMutator,
Review Comment:
Moved method in a26fec3
--
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] Replaced last location mode of 'compaction' with 'none' [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3926:
URL: https://github.com/apache/accumulo/pull/3926#discussion_r1401102733
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1645,7 +1645,10 @@ void getAssignments(SortedMap<TServerInstance,TabletServerStatus> currentStatus,
AssignmentParamsImpl params =
AssignmentParamsImpl.fromThrift(currentStatus, currentTServerGroups,
unassigned.entrySet().stream().collect(HashMap::new,
- (m, e) -> m.put(e.getKey(), e.getValue().getServerInstance()), Map::putAll),
Review Comment:
Honestly, I don't remember. Looking at the code though, it was using the value of the current location, or null. Instead of the last location or null.
--
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