You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/12/28 15:13:42 UTC
[GitHub] [accumulo] ivakegg opened a new pull request, #3142: re #1169: Added a property to specify the location mode for tservers
ivakegg opened a new pull request, #3142:
URL: https://github.com/apache/accumulo/pull/3142
* The last location can now be used for either locality
* or for assignments. If locality mode is chosen, then the last
* location will retain the last major compaction location. If
* assignment mode is chosen, then the last location will retain
* the last assigned location. This in combination with the
* manager.startup.tserver properties will allow the tservers
* to startup in the same location they were last assigned.
--
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] ivakegg commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1387311058
> 4. I'm also curious if you had thought about backporting this to 2.1.
This was actually implemented against 2.1 so that backport is trivial. I also have a branch with the backport to 1.10.x prepared and I have tested that on some internal clusters.
--
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] ctubbsii commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1073281593
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/MetaDataStateStore.java:
##########
@@ -60,9 +62,24 @@ public ClosableIterator<TabletLocationState> iterator() {
public void setLocations(Collection<Assignment> assignments) throws DistributedStoreException {
try (var tabletsMutator = ample.mutateTablets()) {
for (Assignment assignment : assignments) {
- tabletsMutator.mutateTablet(assignment.tablet)
- .putLocation(assignment.server, LocationType.CURRENT)
- .deleteLocation(assignment.server, LocationType.FUTURE).deleteSuspension().mutate();
+ TabletMutator mutation = tabletsMutator.mutateTablet(assignment.tablet);
Review Comment:
I would just change this variable name, so it's not confused for a `Mutation` type object, and so it's consistent with the other places we do this.
```suggestion
TabletMutator tabletMutator = tabletsMutator.mutateTablet(assignment.tablet);
```
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -726,6 +726,14 @@ public enum Property {
"The number of threads on each tablet server available to retrieve"
+ " summary data, that is not currently in cache, from RFiles.",
"2.0.0"),
+ TSERV_LAST_LOCATION_MODE("tserver.last.location.mode", "compaction",
+ PropertyType.LAST_LOCATION_MODE,
+ "Describes how the system will record the 'last' location for tablets, which can be used for assigning them when a cluster restarts."
+ + " If 'compaction' is the mode, then the system will record the location where the tablet's most recent compaction occurred."
+ + " If 'assignment' is the mode, then the most recently assigned location will be recorded."
+ + " Also note that manger.startup.tserver properties might need to be set as well to ensure"
Review Comment:
```suggestion
+ " The manager.startup.tserver properties might also need to be set to ensure"
```
##########
server/base/src/main/java/org/apache/accumulo/server/util/ManagerMetadataUtil.java:
##########
@@ -240,11 +245,15 @@ public static Optional<StoredTabletFile> updateTabletDataFile(ServerContext cont
newFile = Optional.of(newDatafile.insert());
TServerInstance self = getTServerInstance(address, zooLock);
- tablet.putLocation(self, LocationType.LAST);
-
- // remove the old location
- if (lastLocation != null && !lastLocation.equals(self)) {
- tablet.deleteLocation(lastLocation, LocationType.LAST);
+ // if the location mode is 'locality'', then preserve the current compaction location in the
Review Comment:
```suggestion
// if the location mode is 'compaction', then preserve the current compaction location in the
```
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/MetaDataStateStore.java:
##########
@@ -102,6 +119,21 @@ private void unassign(Collection<TabletLocationState> tablets,
for (TabletLocationState tls : tablets) {
TabletMutator tabletMutator = tabletsMutator.mutateTablet(tls.extent);
if (tls.current != null) {
+ // if the location mode is assignment, then preserve the current location in the last
+ // location value
+ if ("assignment"
+ .equals(context.getConfiguration().get(Property.TSERV_LAST_LOCATION_MODE))) {
+ TabletMetadata lastMetadata =
+ ample.readTablet(tls.extent, TabletMetadata.ColumnType.LAST);
+ if (lastMetadata != null && lastMetadata.getLast() != null) {
+ if (!lastMetadata.getLast().equals(tls.current)) {
+ tabletMutator.putLocation(tls.current, LocationType.LAST);
+ tabletMutator.deleteLocation(lastMetadata.getLast(), LocationType.LAST);
+ }
+ } else {
+ tabletMutator.putLocation(tls.current, LocationType.LAST);
+ }
+ }
Review Comment:
If it's always updated when it is assigned, do you need to record it again here?
##########
test/src/main/java/org/apache/accumulo/test/functional/CompactLocationModeIT.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.test.functional;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import java.time.Duration;
+
+import org.apache.accumulo.core.client.AccumuloClient;
+import org.apache.accumulo.core.client.BatchWriter;
+import org.apache.accumulo.core.client.security.tokens.PasswordToken;
+import org.apache.accumulo.core.clientImpl.ClientContext;
+import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.data.Mutation;
+import org.apache.accumulo.core.data.Range;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.metadata.MetadataTable;
+import org.apache.accumulo.core.metadata.TabletLocationState;
+import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection;
+import org.apache.accumulo.core.util.UtilWaitThread;
+import org.apache.accumulo.server.manager.state.MetaDataTableScanner;
+import org.junit.jupiter.api.Test;
+
+public class CompactLocationModeIT extends ConfigurableMacBase {
Review Comment:
Since you're extending ConfigurableMacBase, you should probably implement the configure method. It would be good to set the mode to "compaction" explicitly in that method, in case the default ever changes.
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/ZooTabletStateStore.java:
##########
@@ -134,6 +138,18 @@ public void setLocations(Collection<Assignment> assignments) throws DistributedS
TabletMutator tabletMutator = ample.mutateTablet(assignment.tablet);
tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ if ("assignment".equals(context.getConfiguration().get(Property.TSERV_LAST_LOCATION_MODE))) {
+ TabletMetadata lastMetadata =
+ ample.readTablet(assignment.tablet, TabletMetadata.ColumnType.LAST);
+ if (lastMetadata != null && lastMetadata.getLast() != null) {
+ if (!lastMetadata.getLast().equals(assignment.server)) {
+ tabletMutator.putLocation(assignment.server, LocationType.LAST);
+ tabletMutator.deleteLocation(lastMetadata.getLast(), LocationType.LAST);
+ }
+ } else {
+ tabletMutator.putLocation(assignment.server, LocationType.LAST);
+ }
+ }
Review Comment:
I wonder if this whole block can be put into a method that's common to both state stores. They differ very little.
##########
server/base/src/main/java/org/apache/accumulo/server/util/ManagerMetadataUtil.java:
##########
@@ -205,11 +206,15 @@ public static void replaceDatafiles(ServerContext context, KeyExtent extent,
}
TServerInstance self = getTServerInstance(address, zooLock);
- tablet.putLocation(self, LocationType.LAST);
+ // if the location mode is 'locality'', then preserve the current compaction location in the
Review Comment:
```suggestion
// if the location mode is 'compaction', then preserve the current compaction location in the
```
--
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] ctubbsii commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1059458084
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -274,6 +274,13 @@ public enum Property {
PropertyType.BOOLEAN, "Enables JVM metrics functionality using Micrometer", "2.1.0"),
GENERAL_MICROMETER_FACTORY("general.micrometer.factory", "", PropertyType.CLASSNAME,
"Name of class that implements MeterRegistryFactory", "2.1.0"),
+ GENERAL_LOCATION_MODE("general.location.mode", "locality", PropertyType.LAST_LOCATION_MODE,
Review Comment:
You're right. tserver would make more 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
[GitHub] [accumulo] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1058414039
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -274,6 +274,13 @@ public enum Property {
PropertyType.BOOLEAN, "Enables JVM metrics functionality using Micrometer", "2.1.0"),
GENERAL_MICROMETER_FACTORY("general.micrometer.factory", "", PropertyType.CLASSNAME,
"Name of class that implements MeterRegistryFactory", "2.1.0"),
+ GENERAL_LOCATION_MODE("general.location.mode", "locality", PropertyType.LAST_LOCATION_MODE,
Review Comment:
@ctubbsii Should this be a general property or a tserver 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
[GitHub] [accumulo] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1073740800
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/MetaDataStateStore.java:
##########
@@ -60,9 +62,24 @@ public ClosableIterator<TabletLocationState> iterator() {
public void setLocations(Collection<Assignment> assignments) throws DistributedStoreException {
try (var tabletsMutator = ample.mutateTablets()) {
for (Assignment assignment : assignments) {
- tabletsMutator.mutateTablet(assignment.tablet)
- .putLocation(assignment.server, LocationType.CURRENT)
- .deleteLocation(assignment.server, LocationType.FUTURE).deleteSuspension().mutate();
+ TabletMutator mutation = tabletsMutator.mutateTablet(assignment.tablet);
Review Comment:
noted. I will do 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
[GitHub] [accumulo] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1059474308
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/MetaDataStateStore.java:
##########
@@ -83,25 +91,30 @@ public void setFutureLocations(Collection<Assignment> assignments)
}
@Override
- public void unassign(Collection<TabletLocationState> tablets,
+ public void unassign(ServerContext context, Collection<TabletLocationState> tablets,
Review Comment:
I did that because the ZooTabletStateStore did not have access to the context as it was not constructed as such.
--
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] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1073740117
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/ZooTabletStateStore.java:
##########
@@ -134,6 +138,18 @@ public void setLocations(Collection<Assignment> assignments) throws DistributedS
TabletMutator tabletMutator = ample.mutateTablet(assignment.tablet);
tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ if ("assignment".equals(context.getConfiguration().get(Property.TSERV_LAST_LOCATION_MODE))) {
+ TabletMetadata lastMetadata =
+ ample.readTablet(assignment.tablet, TabletMetadata.ColumnType.LAST);
+ if (lastMetadata != null && lastMetadata.getLast() != null) {
+ if (!lastMetadata.getLast().equals(assignment.server)) {
+ tabletMutator.putLocation(assignment.server, LocationType.LAST);
+ tabletMutator.deleteLocation(lastMetadata.getLast(), LocationType.LAST);
+ }
+ } else {
+ tabletMutator.putLocation(assignment.server, LocationType.LAST);
+ }
+ }
Review Comment:
That I can do. I will work that today.
--
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] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1073742017
##########
test/src/main/java/org/apache/accumulo/test/functional/CompactLocationModeIT.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.test.functional;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import java.time.Duration;
+
+import org.apache.accumulo.core.client.AccumuloClient;
+import org.apache.accumulo.core.client.BatchWriter;
+import org.apache.accumulo.core.client.security.tokens.PasswordToken;
+import org.apache.accumulo.core.clientImpl.ClientContext;
+import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.data.Mutation;
+import org.apache.accumulo.core.data.Range;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.metadata.MetadataTable;
+import org.apache.accumulo.core.metadata.TabletLocationState;
+import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection;
+import org.apache.accumulo.core.util.UtilWaitThread;
+import org.apache.accumulo.server.manager.state.MetaDataTableScanner;
+import org.junit.jupiter.api.Test;
+
+public class CompactLocationModeIT extends ConfigurableMacBase {
Review Comment:
ok, that is reasonable. Thanks.
--
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] ivakegg commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1387306123
> 2. I'm also curious what you thought about how it should work if the value were to be changed in the ZooKeeper system config while a system was running.
I update the last location on both the loading of the tablet and on the unloading of the tablet. This allows one to change the mode while the system is up and running and still have the last location updated when the system is taken down.
--
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] ddanielr commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ddanielr commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1367515192
> In assignment mode, this will set the last location to the currently assigned location when the tablet is unhosted. I am wondering if I should set the last location when the tablet is loaded as well or instead. The current implementation requires an orderly shutdown. I am thinking I would like this to work even on a non-orderly shutdown.
When you mention orderly shutdown vs non-orderly, what are you defining as a non-orderly shutdown?
--
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-turner commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
keith-turner commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1368091960
I was thinking through the possibility of having multiple last columns, like `lastAssigned`, `lastCompacted`, etc. All that information could be made available to the balancer. Then I started thinking about external compactions and if they would set the `lastCompacted` location. That prompted me to ask about external compactions.
--
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] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1059473711
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -726,6 +726,14 @@ public enum Property {
"The number of threads on each tablet server available to retrieve"
+ " summary data, that is not currently in cache, from RFiles.",
"2.0.0"),
+ TSERV_LAST_LOCATION_MODE("tserver.last.location.mode", "compact", PropertyType.LAST_LOCATION_MODE,
+ "Describes how the system will assign tablets initially by defining how the 'last' location is updated."
+ + " If 'compact' is the mode, then the system will assign tablets based on the data locality (e.g. the last compaction location)."
+ + " If 'assign' is the mode, then tablets will be initially assigned to the last place they were assigned."
+ + " If 'unload' is the mode, then tablets will be initially assigned to the last place they were unloaded from (i.e. requires a clean shutdown)."
Review Comment:
ok, that is reasonable argument. I was briefly thinking that one would want only updating of the last location on unload. This would allow a scan of the accumulo metadata to see what the discrepencies between the last and current locations.
--
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] ctubbsii commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1405328601
Committed to 2.1 branch as 7f19f96914ed12614bbd2d6b24a88d322e49b006 and merged forward into main branch
--
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] ivakegg commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1387308147
> 3. Also, the property name is a tserver property... but since the locations are updated via Ample, it's not clear that the tserver is the only one that exclusively updates them. I wonder if the manager might update the current/last location, and if the property is still appropriately named in that case.
I am pretty sure only the tserver is updating the last location. That holds for both modes. Also I want to be able to change this property while the system is running.
--
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] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1059087306
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -274,6 +274,13 @@ public enum Property {
PropertyType.BOOLEAN, "Enables JVM metrics functionality using Micrometer", "2.1.0"),
GENERAL_MICROMETER_FACTORY("general.micrometer.factory", "", PropertyType.CLASSNAME,
"Name of class that implements MeterRegistryFactory", "2.1.0"),
+ GENERAL_LOCATION_MODE("general.location.mode", "locality", PropertyType.LAST_LOCATION_MODE,
Review Comment:
So it appears that a general property cannot be set in zookeeper. I am leaning towards changing this to a tserver property unless you have a reason why not to.
--
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] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1059527511
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/ZooTabletStateStore.java:
##########
@@ -134,6 +138,18 @@ public void setLocations(Collection<Assignment> assignments) throws DistributedS
TabletMutator tabletMutator = ample.mutateTablet(assignment.tablet);
tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ if ("assignment".equals(context.getConfiguration().get(Property.TSERV_LAST_LOCATION_MODE))) {
+ TabletMetadata lastMetadata =
Review Comment:
This may not be needed in a zoo tablet state store as I believe there may be and only 1 possible 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
[GitHub] [accumulo] ivakegg commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1368086838
> With the new support for external compactions, does that make the last compaction location may be a bit less relevant?
Yes I believe it does. The concept of locality goes out the windiow with external compactions.
--
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] ivakegg commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1367456136
In assignment mode, this will set the last location to the currently assigned location when the tablet is unhosted. I am wondering if I should set the last location when the tablet is loaded as well or instead. The current implementation requires an orderly shutdown. I am thinking I would like this to work even on a non-orderly shutdown.
--
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] ivakegg commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by "ivakegg (via GitHub)" <gi...@apache.org>.
ivakegg commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1405242835
@ctubbsii Are you now ok with this PR or do you hvae additional concerns? Do you want additional PRs to go against 2.1 and 1.10 ?
--
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] ivakegg commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1367500203
Now I am thinking of 3 modes for location: locality, assignment, and unload. Also maybe compaction instead of locality, and maybe last.location.mode instead of just location mode. Any thoughts out there?
--
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] ctubbsii commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1059458662
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -726,6 +726,14 @@ public enum Property {
"The number of threads on each tablet server available to retrieve"
+ " summary data, that is not currently in cache, from RFiles.",
"2.0.0"),
+ TSERV_LAST_LOCATION_MODE("tserver.last.location.mode", "compact", PropertyType.LAST_LOCATION_MODE,
+ "Describes how the system will assign tablets initially by defining how the 'last' location is updated."
Review Comment:
```suggestion
"Describes how the system will record the 'last' location for tablets, which can be used for assigning them when a cluster restarts."
```
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/MetaDataStateStore.java:
##########
@@ -83,25 +91,30 @@ public void setFutureLocations(Collection<Assignment> assignments)
}
@Override
- public void unassign(Collection<TabletLocationState> tablets,
+ public void unassign(ServerContext context, Collection<TabletLocationState> tablets,
Review Comment:
I think we probably don't need to keep passing in ServerContext to each instance method if we just add it to the constructor.
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -726,6 +726,14 @@ public enum Property {
"The number of threads on each tablet server available to retrieve"
+ " summary data, that is not currently in cache, from RFiles.",
"2.0.0"),
+ TSERV_LAST_LOCATION_MODE("tserver.last.location.mode", "compact", PropertyType.LAST_LOCATION_MODE,
+ "Describes how the system will assign tablets initially by defining how the 'last' location is updated."
+ + " If 'compact' is the mode, then the system will assign tablets based on the data locality (e.g. the last compaction location)."
+ + " If 'assign' is the mode, then tablets will be initially assigned to the last place they were assigned."
Review Comment:
```suggestion
+ " If 'assign' is the mode, then the most recently assigned location will be recorded."
```
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -726,6 +726,14 @@ public enum Property {
"The number of threads on each tablet server available to retrieve"
+ " summary data, that is not currently in cache, from RFiles.",
"2.0.0"),
+ TSERV_LAST_LOCATION_MODE("tserver.last.location.mode", "compact", PropertyType.LAST_LOCATION_MODE,
+ "Describes how the system will assign tablets initially by defining how the 'last' location is updated."
+ + " If 'compact' is the mode, then the system will assign tablets based on the data locality (e.g. the last compaction location)."
+ + " If 'assign' is the mode, then tablets will be initially assigned to the last place they were assigned."
+ + " If 'unload' is the mode, then tablets will be initially assigned to the last place they were unloaded from (i.e. requires a clean shutdown)."
Review Comment:
```suggestion
+ " If 'unload' is the mode, then tablets will be initially assigned to the last place they were unloaded from (i.e. requires a clean shutdown)."
```
I'm not sure we need this one at all. This would require a clean shut down in order to freeze the current assignments. And, in the case of that clean shut down, this would be no different than the `assign` mode. If there is not a clean shutdown, then the field would merely be blank... which isn't helpful.
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -726,6 +726,14 @@ public enum Property {
"The number of threads on each tablet server available to retrieve"
+ " summary data, that is not currently in cache, from RFiles.",
"2.0.0"),
+ TSERV_LAST_LOCATION_MODE("tserver.last.location.mode", "compact", PropertyType.LAST_LOCATION_MODE,
+ "Describes how the system will assign tablets initially by defining how the 'last' location is updated."
+ + " If 'compact' is the mode, then the system will assign tablets based on the data locality (e.g. the last compaction location)."
Review Comment:
```suggestion
+ " If 'compact' is the mode, then the system will record the location where the tablet's most recent compaction occurred."
```
--
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] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1059473711
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -726,6 +726,14 @@ public enum Property {
"The number of threads on each tablet server available to retrieve"
+ " summary data, that is not currently in cache, from RFiles.",
"2.0.0"),
+ TSERV_LAST_LOCATION_MODE("tserver.last.location.mode", "compact", PropertyType.LAST_LOCATION_MODE,
+ "Describes how the system will assign tablets initially by defining how the 'last' location is updated."
+ + " If 'compact' is the mode, then the system will assign tablets based on the data locality (e.g. the last compaction location)."
+ + " If 'assign' is the mode, then tablets will be initially assigned to the last place they were assigned."
+ + " If 'unload' is the mode, then tablets will be initially assigned to the last place they were unloaded from (i.e. requires a clean shutdown)."
Review Comment:
ok, that is reasonable argument. I was briefly thinking that one would want only one or the other to be able to see the discrepency between the last location and the new locations when scanning the metadata and using the "unload" mode.
--
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] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1059474308
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/MetaDataStateStore.java:
##########
@@ -83,25 +91,30 @@ public void setFutureLocations(Collection<Assignment> assignments)
}
@Override
- public void unassign(Collection<TabletLocationState> tablets,
+ public void unassign(ServerContext context, Collection<TabletLocationState> tablets,
Review Comment:
I did that because the RootDataStateStore did not have access to the context as it was not constructed as such.
--
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] ivakegg commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1368049753
I am finding a bunch of issues with the implementation:
1) With assign or unload mode, I am getting multiple last locations (the previous last location is not being deleted). What's worse is that at the place where these updates are being made, the previous last location is not available.
2) I need to rework how the configuration is available for the assign/unload mode.
3) The unload mode will probably be dropped.
Turning this into a draft until I can resolve these issues.
--
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] ctubbsii merged pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #3142:
URL: https://github.com/apache/accumulo/pull/3142
--
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] ivakegg commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by "ivakegg (via GitHub)" <gi...@apache.org>.
ivakegg commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1405487315
@ctubbsii OK, thank you for merging. I can keep 1.10 backport exclusively in our own repo.
--
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] ivakegg commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1367898920
> > In assignment mode, this will set the last location to the currently assigned location when the tablet is unhosted. I am wondering if I should set the last location when the tablet is loaded as well or instead. The current implementation requires an orderly shutdown. I am thinking I would like this to work even on a non-orderly shutdown.
>
> When you mention orderly shutdown vs non-orderly, what are you defining as a non-orderly shutdown?
A non-orderly shutdown is when you force all of the tservers to be killed (kill <pid>) in which case the tservers will not unload the tablets first and hence the last location would not be updated in "assign" mode.
--
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] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1062808474
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/ZooTabletStateStore.java:
##########
@@ -134,6 +138,18 @@ public void setLocations(Collection<Assignment> assignments) throws DistributedS
TabletMutator tabletMutator = ample.mutateTablet(assignment.tablet);
tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ if ("assignment".equals(context.getConfiguration().get(Property.TSERV_LAST_LOCATION_MODE))) {
+ TabletMetadata lastMetadata =
Review Comment:
nevermind. It appears that we are storing all cf, cq, and values now as a json blob.
--
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-turner commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
keith-turner commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1368085098
With the new support for external compactions, does that make the last compaction location may be a bit less relevant?
--
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] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1059516799
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/MetaDataStateStore.java:
##########
@@ -83,25 +91,30 @@ public void setFutureLocations(Collection<Assignment> assignments)
}
@Override
- public void unassign(Collection<TabletLocationState> tablets,
+ public void unassign(ServerContext context, Collection<TabletLocationState> tablets,
Review Comment:
Updated to construct the ZooTabletStateStore with the context. Reset the TabletStateStore interface to what it was originally.
--
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] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1073833133
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/ZooTabletStateStore.java:
##########
@@ -134,6 +138,18 @@ public void setLocations(Collection<Assignment> assignments) throws DistributedS
TabletMutator tabletMutator = ample.mutateTablet(assignment.tablet);
tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ if ("assignment".equals(context.getConfiguration().get(Property.TSERV_LAST_LOCATION_MODE))) {
+ TabletMetadata lastMetadata =
+ ample.readTablet(assignment.tablet, TabletMetadata.ColumnType.LAST);
+ if (lastMetadata != null && lastMetadata.getLast() != null) {
+ if (!lastMetadata.getLast().equals(assignment.server)) {
+ tabletMutator.putLocation(assignment.server, LocationType.LAST);
+ tabletMutator.deleteLocation(lastMetadata.getLast(), LocationType.LAST);
+ }
+ } else {
+ tabletMutator.putLocation(assignment.server, LocationType.LAST);
+ }
+ }
Review Comment:
I refactored the location updating into methods in the ManagerMetadataUtil class. If you want them located elsewhere or in their own class just let me know.
--
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] ivakegg commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1387304543
> 1. What behavior do you expect if there was a crash on assignment, or a crash/kill that circumvented an unassignment? I was wondering if the behavior in these edge cases for the 'assignment' mode made sense as you've implemented them here.
I update the last location on both the loading of the tablet and on the unloading of the tablet. Hence on a crash/kill the last location will still have been updated. Also this allows one to change the mode while the system is up and running and still have the last location updated when the system is taken down. Perhaps I don't quite understand this question however.
--
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] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1073739091
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/MetaDataStateStore.java:
##########
@@ -102,6 +119,21 @@ private void unassign(Collection<TabletLocationState> tablets,
for (TabletLocationState tls : tablets) {
TabletMutator tabletMutator = tabletsMutator.mutateTablet(tls.extent);
if (tls.current != null) {
+ // if the location mode is assignment, then preserve the current location in the last
+ // location value
+ if ("assignment"
+ .equals(context.getConfiguration().get(Property.TSERV_LAST_LOCATION_MODE))) {
+ TabletMetadata lastMetadata =
+ ample.readTablet(tls.extent, TabletMetadata.ColumnType.LAST);
+ if (lastMetadata != null && lastMetadata.getLast() != null) {
+ if (!lastMetadata.getLast().equals(tls.current)) {
+ tabletMutator.putLocation(tls.current, LocationType.LAST);
+ tabletMutator.deleteLocation(lastMetadata.getLast(), LocationType.LAST);
+ }
+ } else {
+ tabletMutator.putLocation(tls.current, LocationType.LAST);
+ }
+ }
Review Comment:
Yes I do. This will allow me to change from compaction to assignment on a running system and still get the benefit of the last locations being updated appropriately when the system is shutdown.
--
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] ivakegg commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
ivakegg commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1081352463
##########
server/base/src/main/java/org/apache/accumulo/server/util/ManagerMetadataUtil.java:
##########
@@ -256,4 +247,64 @@ public static Optional<StoredTabletFile> updateTabletDataFile(ServerContext cont
tablet.mutate();
return newFile;
}
+
+ /**
+ * 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 ample The metadata persistence layer
+ * @param tabletMutator The mutator being built
+ * @param extent The tablet extent
+ * @param location The new location
+ */
+ public static void updateLastForAssignmentMode(ClientContext context, Ample ample,
+ Ample.TabletMutator tabletMutator, KeyExtent extent, TServerInstance location) {
+ // if the location mode is assignment, then preserve the current location in the last
+ // location value
+ if ("assignment".equals(context.getConfiguration().get(Property.TSERV_LAST_LOCATION_MODE))) {
+ TabletMetadata lastMetadata = ample.readTablet(extent, TabletMetadata.ColumnType.LAST);
+ TServerInstance lastLocation = (lastMetadata == null ? null : lastMetadata.getLast());
+ ManagerMetadataUtil.updateLast(tabletMutator, lastLocation, location);
+ }
+ }
+
+ /**
+ * Update the last location if the location mode is "compaction". 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 lastLocation The last location
+ * @param address The server address
+ * @param zooLock The zookeeper lock
+ */
+ public static void updateLastForCompactionMode(ClientContext context, TabletMutator tabletMutator,
+ TServerInstance lastLocation, String address, ServiceLock zooLock) {
+ // if the location mode is 'compaction', then preserve the current compaction location in the
+ // last location value
+ if ("compaction".equals(context.getConfiguration().get(Property.TSERV_LAST_LOCATION_MODE))) {
+ TServerInstance newLocation = getTServerInstance(address, zooLock);
+ updateLast(tabletMutator, lastLocation, newLocation);
+ }
+ }
+
+ /**
+ * Update the last location, deleting the previous location if needed
+ *
+ * @param tabletMutator The mutator being built
+ * @param lastLocation The last location (may be null)
+ * @param newLocation The new location
+ */
+ public static void updateLast(TabletMutator tabletMutator, TServerInstance lastLocation,
+ TServerInstance newLocation) {
+ if (lastLocation != null) {
+ if (!lastLocation.equals(newLocation)) {
+ tabletMutator.deleteLocation(lastLocation, LocationType.LAST);
+ tabletMutator.putLocation(newLocation, LocationType.LAST);
+ }
+ } else {
+ tabletMutator.putLocation(newLocation, LocationType.LAST);
+ }
Review Comment:
But I did not want to "putLocation" if the lastLocation is equal to the newLocation. This suggestion is too simple.
--
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] ctubbsii commented on pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#issuecomment-1405281252
@ivakegg asked:
> @ctubbsii Are you now ok with this PR or do you hvae additional concerns? Do you want additional PRs to go against 2.1 and 1.10 ?
I don't have any other concerns, and I'm okay with this for 2.1.1, but the PR will need to be remade for that, and the since version for the property will need to be updated. For 1.10, I don't think it's worth it... the code is very different and it would require changing the "since" information to the property from a simple "2.1.1" to a more complicated "1.10.3, but not 2.x before 2.1.1, then from 2.1.1 onward". It's just very confusing to manage expectations for users when we try to backport new features. I'm okay with 2.1.1 only because there's no harm if the user has this set for 2.1.0... the property would just be ignored... and because there's no API changes or substantive behavior changes, just an optimization knob.
To add to 2.1.1, I'm going to create a temporary working branch, switch the base branch for this PR to that, merge it, then cherry-pick it to 2.1 and update the references to 3.0.0 to 2.1.1. Then I'll merge it forward to the main branch and get rid of the temporary working branch. That way, it's just easier on you and you don't need to create a new PR, and we can get this merged in sooner to 2.1. If 1.10 is desired, we'll need a new PR... to backport, and the merge forward will be a little complicated... but as I said, I do not think we should do 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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3142: re #1169: Added a property to specify the location mode for tservers
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3142:
URL: https://github.com/apache/accumulo/pull/3142#discussion_r1081157753
##########
server/base/src/main/java/org/apache/accumulo/server/util/ManagerMetadataUtil.java:
##########
@@ -256,4 +247,64 @@ public static Optional<StoredTabletFile> updateTabletDataFile(ServerContext cont
tablet.mutate();
return newFile;
}
+
+ /**
+ * 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 ample The metadata persistence layer
+ * @param tabletMutator The mutator being built
+ * @param extent The tablet extent
+ * @param location The new location
+ */
+ public static void updateLastForAssignmentMode(ClientContext context, Ample ample,
+ Ample.TabletMutator tabletMutator, KeyExtent extent, TServerInstance location) {
+ // if the location mode is assignment, then preserve the current location in the last
+ // location value
+ if ("assignment".equals(context.getConfiguration().get(Property.TSERV_LAST_LOCATION_MODE))) {
+ TabletMetadata lastMetadata = ample.readTablet(extent, TabletMetadata.ColumnType.LAST);
+ TServerInstance lastLocation = (lastMetadata == null ? null : lastMetadata.getLast());
+ ManagerMetadataUtil.updateLast(tabletMutator, lastLocation, location);
+ }
+ }
+
+ /**
+ * Update the last location if the location mode is "compaction". 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 lastLocation The last location
+ * @param address The server address
+ * @param zooLock The zookeeper lock
+ */
+ public static void updateLastForCompactionMode(ClientContext context, TabletMutator tabletMutator,
+ TServerInstance lastLocation, String address, ServiceLock zooLock) {
+ // if the location mode is 'compaction', then preserve the current compaction location in the
+ // last location value
+ if ("compaction".equals(context.getConfiguration().get(Property.TSERV_LAST_LOCATION_MODE))) {
+ TServerInstance newLocation = getTServerInstance(address, zooLock);
+ updateLast(tabletMutator, lastLocation, newLocation);
+ }
+ }
+
+ /**
+ * Update the last location, deleting the previous location if needed
+ *
+ * @param tabletMutator The mutator being built
+ * @param lastLocation The last location (may be null)
+ * @param newLocation The new location
+ */
+ public static void updateLast(TabletMutator tabletMutator, TServerInstance lastLocation,
+ TServerInstance newLocation) {
+ if (lastLocation != null) {
+ if (!lastLocation.equals(newLocation)) {
+ tabletMutator.deleteLocation(lastLocation, LocationType.LAST);
+ tabletMutator.putLocation(newLocation, LocationType.LAST);
+ }
+ } else {
+ tabletMutator.putLocation(newLocation, LocationType.LAST);
+ }
Review Comment:
could be simplified:
```suggestion
if (lastLocation != null && !lastLocation.equals(newLocation)) {
tabletMutator.deleteLocation(lastLocation, LocationType.LAST);
}
tabletMutator.putLocation(newLocation, LocationType.LAST);
```
--
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