ctubbsii commented on code in PR #3139:
URL: https://github.com/apache/accumulo/pull/3139#discussion_r1056739016
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -851,6 +851,14 @@ public enum Property {
+ " the tablet server this duration to revive before reassigning its
tablets"
+ " to other tablet servers."),
+ TABLE_LOCATION_MODE("table.location.mode", "locality",
PropertyType.LAST_LOCATION_MODE,
+ "Describes how the system will assign tablets initially."
+ + " If 'locality' is the mode, then the system will assign tablets
based on the data locality (e.g. the last major compaction location)."
+ + " If 'assignment' is the mode, then tablets will be initially
assigned to the last place they were assigned which could be"
+ + " different then where they were last compacted given balancing
activities."
+ + " Also note that master.startup.tserver properties might need to
be set as well to ensure"
Review Comment:
Note on merging this forward into 2.1: the term `master` has changed to
`manager` in newer versions, and this should be updated when this is merged
forward.
```suggestion
+ " Also note that master.startup.tserver properties might need to
be set as well to ensure"
```
##########
server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java:
##########
@@ -139,6 +140,11 @@ public void suspend(Collection<TabletLocationState>
tablets,
for (TabletLocationState tls : tablets) {
Mutation m = new Mutation(tls.extent.getMetadataEntry());
if (tls.current != null) {
+ // if the location more is assignment, then preserve the current
location in the last
+ // location value
+ if
("assignment".equals(context.getConfiguration().get(Property.TABLE_LOCATION_MODE)))
{
+ tls.current.putLastLocation(m);
+ }
Review Comment:
This is definitely reading it from the system configuration, not the
per-table configuration, so it should be a `general.` property, not a `table.`
one.
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -851,6 +851,14 @@ public enum Property {
+ " the tablet server this duration to revive before reassigning its
tablets"
+ " to other tablet servers."),
+ TABLE_LOCATION_MODE("table.location.mode", "locality",
PropertyType.LAST_LOCATION_MODE,
Review Comment:
Do we want this to be a per-table property, or a global property?
##########
server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java:
##########
@@ -319,21 +324,30 @@ private static void updateRootTabletDataFile(KeyExtent
extent, FileRef path, Fil
*
* @return A Mutation to update a tablet from the given information
*/
- private static Mutation getUpdateForTabletDataFile(KeyExtent extent, FileRef
path,
- FileRef mergeFile, DataFileValue dfv, String time, Set<FileRef>
filesInUseByScans,
- String address, ZooLock zooLock, Set<String> unusedWalLogs,
TServerInstance lastLocation,
- long flushId) {
+ private static Mutation getUpdateForTabletDataFile(ClientContext context,
KeyExtent extent,
+ FileRef path, FileRef mergeFile, DataFileValue dfv, String time,
+ Set<FileRef> filesInUseByScans, String address, ZooLock zooLock,
Set<String> unusedWalLogs,
+ TServerInstance lastLocation, long flushId) {
Mutation m = new Mutation(extent.getMetadataEntry());
if (dfv.getNumEntries() > 0) {
m.put(DataFileColumnFamily.NAME, path.meta(), new Value(dfv.encode()));
TabletsSection.ServerColumnFamily.TIME_COLUMN.put(m, new
Value(time.getBytes(UTF_8)));
- // stuff in this location
- TServerInstance self = getTServerInstance(address, zooLock);
- self.putLastLocation(m);
- // erase the old location
- if (lastLocation != null && !lastLocation.equals(self))
- lastLocation.clearLastLocation(m);
+
+ // if the location mode is 'locality'', then preserve the current
compaction location in the
+ // last location value
+ if
("locality".equals(context.getConfiguration().get(Property.TABLE_LOCATION_MODE)))
{
+ // stuff in this location
+ TServerInstance self = getTServerInstance(address, zooLock);
+ self.putLastLocation(m);
+ // erase the old location
+ if (lastLocation != null && !lastLocation.equals(self))
+ // @TODO: this does not make any sense to me as this will simply
clear out the location we
+ // just set a couple lines previously
+ // clearLastLocation will clear the "last" location no matter what
the value is
+ lastLocation.clearLastLocation(m);
Review Comment:
This is setting the last location on a completely different tserver. It's
updating the mutation by putting the new tserver as the last location, and
clearing the previous tserver as the last location. One is a put, and one is a
delete, on two separate metadata columns. They have the same column family
("last"), but different column qualifiers (the tserver hostname). We check that
they aren't the same, so we're not adding a `delete` to the same mutation that
we just added a `put` for the same column.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
