You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ddanielr (via GitHub)" <gi...@apache.org> on 2023/04/04 02:02:47 UTC
[GitHub] [accumulo] ddanielr opened a new pull request, #3274: WIP: Feature/3208 consolidate metadata code
ddanielr opened a new pull request, #3274:
URL: https://github.com/apache/accumulo/pull/3274
This PR is part of #3208 with the intended goal to be removal of the `ManagerMetadataUtil` and `MetadataTableUtil` classes to consolidate metadata operations.
The first part of the `ManagerMetadataUtil` class removal was completed in #3255.
Currently, all methods have been refactored out from ManagerMetadataUtil and that class has been removed.
However, these changes are currently failing the `org.apache.accumulo.server.manager.state.RootTabletStateStoreTest.testRootTabletStateStore` test due to a `NullPointerException` being thrown in `MockServerContext.java`
After poking at it, I've confirmed that the error is happening before my code changes in `RootTabletStateStoreTest`.
It seems that the exception is thrown when attempting to access configuration properties that should be set in the mock object.
`server/base/src/test/java/org/apache/accumulo/server/MockServerContext.java:47`
```
expect(context.getConfiguration().get(Property.INSTANCE_VOLUMES)).andReturn("file:///")
.anyTimes();
```
Any help would be appreciated.
--
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 a diff in pull request #3274: WIP: Feature/3208 consolidate metadata code
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3274:
URL: https://github.com/apache/accumulo/pull/3274#discussion_r1157644809
##########
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 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.
--
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 a diff in pull request #3274: WIP: Feature/3208 consolidate metadata code
Posted by "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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3274: WIP: Feature/3208 consolidate metadata code
Posted by "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
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3274: WIP: Feature/3208 consolidate metadata code
Posted by "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 abstraction 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.
--
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