keith-turner commented on code in PR #3274:
URL: https://github.com/apache/accumulo/pull/3274#discussion_r1157637939
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -182,6 +183,18 @@ default TabletsMutator mutateTablets() {
throw new UnsupportedOperationException();
}
+ /**
+ * Use this method to fix or rollback an incomplete split operation.
+ *
+ * @param tabletMetadata tablet meta to fix
+ * @param lock Zookeeper lock to pass to the finishSplit operation.
+ * @return KeyExtent of a fixed split
+ */
+ default KeyExtent fixSplit(TabletMetadata tabletMetadata, ServiceLock lock)
Review Comment:
I see Ample as an abstracting layer for reading and writing accumulo
metadata. The implementation of this method does more than simply read and
write, it does read+manipulate+write. Its only used by one class
(AssignmentHandler) and a test. We could place this code as a static method in
AssignmentHandler instead of here in Ample.
##########
server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java:
##########
@@ -388,4 +400,80 @@ public void
deleteScanServerFileReferences(Collection<ScanServerRefTabletFile> r
}
}
+ @Override
+ public KeyExtent fixSplit(TabletMetadata meta, ServiceLock lock) throws
AccumuloException {
+ log.info("Incomplete split {} attempting to fix", meta.getExtent());
+
+ if (meta.getSplitRatio() == null) {
+ throw new IllegalArgumentException(
+ "Metadata entry does not have split ratio (" + meta.getExtent() +
")");
+ }
+
+ if (meta.getTime() == null) {
+ throw new IllegalArgumentException(
+ "Metadata entry does not have time (" + meta.getExtent() + ")");
+ }
+ return fixSplit(meta.getTableId(), meta.getExtent().toMetaRow(),
meta.getPrevEndRow(),
+ meta.getOldPrevEndRow(), meta.getSplitRatio(), lock);
+ }
+
+ private KeyExtent fixSplit(TableId tableId, Text metadataEntry, Text
metadataPrevEndRow,
+ Text oper, double splitRatio, ServiceLock lock) throws AccumuloException
{
+ if (metadataPrevEndRow == null) {
+ // something is wrong, this should not happen... if a tablet is split,
it will always have a
+ // prev end row....
+ throw new AccumuloException(
+ "Split tablet does not have prev end row, something is amiss, extent
= " + metadataEntry);
+ }
+
+ // check to see if prev tablet exist in metadata tablet
+ Key prevRowKey =
+ new Key(new Text(MetadataSchema.TabletsSection.encodeRow(tableId,
metadataPrevEndRow)));
+
+ try (
+ Scanner scanner = context.createScanner(DataLevel.USER.metaTable(),
Authorizations.EMPTY)) {
Review Comment:
Its not directly related to this PRs purpose so feel free to ignore this
comment. Looking at this code, it would be nice to update it to use Ample to
read metadata.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -243,6 +256,10 @@ public interface TabletsMutator extends AutoCloseable {
* Interface for changing a tablets persistent data.
*/
interface TabletMutator {
+ TabletMutator updateLastForAssignmentMode(KeyExtent extent,
TServerInstance location);
Review Comment:
I think of tablet mutator as some something the helps build a mutation for
updating the Accumulo metadata table. The implementation of this method reads
the tablets while building the mutation, which could have race conditions
depending on the context. These potential race conditions and the fact that it
read the tablet need to be documented here or the more complicated logic of
read,modify,update code should be located elsewhere and TabletMutator only does
the write portion.
##########
server/base/src/test/java/org/apache/accumulo/server/MockServerContext.java:
##########
@@ -40,7 +40,12 @@ public static ServerContext get() {
ServerContext context = EasyMock.createMock(ServerContext.class);
ConfigurationCopy conf = new
ConfigurationCopy(DefaultConfiguration.getInstance());
conf.set(Property.INSTANCE_VOLUMES, "file:///");
+ // Added the new expected property value
+ conf.set(Property.TSERV_LAST_LOCATION_MODE, "assignment");
expect(context.getConfiguration()).andReturn(conf).anyTimes();
+ // Null pointer pops when attempting to access the property in the context.
+
expect(context.getConfiguration().get(Property.INSTANCE_VOLUMES)).andReturn("file:///")
+ .anyTimes();
Review Comment:
Seems like this could be removed because its set on the conf object.
```suggestion
```
In general I think `context.getConfiguration()` will return null until
`EasyMock.replay(context)` is called.
##########
server/base/src/test/java/org/apache/accumulo/server/manager/state/RootTabletStateStoreTest.java:
##########
@@ -92,6 +93,10 @@ public void mutate() {
public void testRootTabletStateStore() throws DistributedStoreException {
ServerContext context = MockServerContext.get();
expect(context.getAmple()).andReturn(new TestAmple()).anyTimes();
+
expect(context.getConfiguration().isPropertySet(Property.TSERV_LAST_LOCATION_MODE))
+ .andReturn(true).once();
+ expect(context.getConfiguration().get((Property.TSERV_LAST_LOCATION_MODE)))
+ .andReturn(Property.TSERV_LAST_LOCATION_MODE.getDefaultValue()).once();
Review Comment:
I looked at a little bit and I think the following changes could improve the
situation, but not sure if its workable.
* Add a metthod called `MockServerContext.create(Map<String,String> props)`
that creates a server context with the requested system props.
* Add a TestAmple constructor that takes server context.
Then may be able to do the following.
```suggestion
public void testRootTabletStateStore() throws DistributedStoreException {
ServerContext context =
MockServerContext.create(Map.of(Property.TSERV_LAST_LOCATION_MODE,
Property.TSERV_LAST_LOCATION_MODE.getDefaultValue()));
expect(context.getAmple()).andReturn(new TestAmple(context)).anyTimes();
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/DatafileManager.java:
##########
@@ -510,11 +513,38 @@ Optional<StoredTabletFile>
bringMajorCompactionOnline(Set<StoredTabletFile> oldD
if (!filesInUseByScans.isEmpty()) {
log.debug("Adding scan refs to metadata {} {}", extent,
filesInUseByScans);
}
- ManagerMetadataUtil.replaceDatafiles(tablet.getContext(), extent,
oldDatafiles,
- filesInUseByScans, newFile, compactionIdToWrite, dfv,
- tablet.getTabletServer().getClientAddressString(), lastLocation,
- tablet.getTabletServer().getLock(), ecid);
- tablet.setLastCompactionID(compactionIdToWrite);
+
Review Comment:
Was this method inlined? Instead of inlining could just move the method
replaceDatafiles to this class. Thinking move instead of inline is a bit
better because the bringMajorCompactionOnline method is already a bit long.
--
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]
