[GitHub] [accumulo] keith-turner commented on a diff in pull request #3274: WIP: Feature/3208 consolidate metadata code

["keith-turner (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org