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

["dlmarion (via GitHub)" <gi...@apache.org>]

dlmarion commented on code in PR #3274:
URL: https://github.com/apache/accumulo/pull/3274#discussion_r1158646240


##########
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);
+
+    TabletMutator updateLastForCompactionMode(Location location, TServerInstance serverInstance);

Review Comment:
   I wonder if the same concerns that Keith has for the `updateLastForAssignmentMode` apply here.



##########
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:
   `updateLastForAssignmentMode` is only called by ZooTabletStateStore and MetaDataStateStore. I wonder if this method could be moved to the TabletStateStore interface as a static method.



##########
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:
   ```
   expect(context.getConfiguration()).andReturn(conf).anyTimes();
   ```
   
   will return the ConfigurationCopy object (which is not a Mock object). So, you don't need to mock calls to the Configuration object, but the Configuration object does need to be populated with the properties you are looking for.  In this case it is, on line 42. I agree with @keith-turner , I believe that lines 46 - 48 can be removed.



-- 
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