Skip to content

Commit

Permalink
Ensure getTabletState() in TabletMetadata does not allow a null extent (
Browse files Browse the repository at this point in the history
#4438)

Previously the private reference for the extent inside TabletMetadata
was passed to the TabletLocationState constructor inside of
getTabletState(). The extent may not have been loaded at this point as
it is lazy loaded by the getExtent() method and requires PREV_ROW to
have been fetched. This change now uses the getter to make sure we do
not inadvertently pass a null extent which is invalid.
  • Loading branch information
cshannon authored Apr 6, 2024
1 parent 01ccb96 commit 90bcb46
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import java.util.Collection;
import java.util.Collections;
import java.util.Objects;
import java.util.Set;

import org.apache.accumulo.core.dataImpl.KeyExtent;
Expand Down Expand Up @@ -58,7 +59,7 @@ public Text getEncodedEndRow() {
public TabletLocationState(KeyExtent extent, Location future, Location current, Location last,
SuspendingTServer suspend, Collection<Collection<String>> walogs, boolean chopped)
throws BadLocationStateException {
this.extent = extent;
this.extent = Objects.requireNonNull(extent);
this.future = validateLocation(future, TabletMetadata.LocationType.FUTURE);
this.current = validateLocation(current, TabletMetadata.LocationType.CURRENT);
this.last = validateLocation(last, TabletMetadata.LocationType.LAST);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,9 @@ public TabletState getTabletState(Set<TServerInstance> liveTServers) {
future = location;
}
// only care about the state so don't need walogs and chopped params
var tls = new TabletLocationState(extent, future, current, last, suspend, null, false);
// Use getExtent() when passing the extent as the private reference may not have been
// initialized yet. This will also ensure PREV_ROW was fetched
var tls = new TabletLocationState(getExtent(), future, current, last, suspend, null, false);
return tls.getState(liveTServers);
} catch (TabletLocationState.BadLocationStateException blse) {
throw new IllegalArgumentException("Error creating TabletLocationState", blse);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import static org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.ServerColumnFamily.TIME_COLUMN;
import static org.apache.accumulo.core.metadata.schema.TabletMetadata.ColumnType.LAST;
import static org.apache.accumulo.core.metadata.schema.TabletMetadata.ColumnType.LOCATION;
import static org.apache.accumulo.core.metadata.schema.TabletMetadata.ColumnType.PREV_ROW;
import static org.apache.accumulo.core.metadata.schema.TabletMetadata.ColumnType.SUSPEND;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
Expand Down Expand Up @@ -196,6 +197,13 @@ public void testLocationStates() {
.put(ser1.getHostPort());
SortedMap<Key,Value> rowMap = toRowMap(mutation);

// PREV_ROW was not fetched
final var missingPrevRow =
TabletMetadata.convertRow(rowMap.entrySet().iterator(), colsToFetch, false);
assertThrows(IllegalStateException.class, () -> missingPrevRow.getTabletState(tservers));

// This should now work as PREV_ROW has been included
colsToFetch = EnumSet.of(LOCATION, LAST, SUSPEND, PREV_ROW);
TabletMetadata tm = TabletMetadata.convertRow(rowMap.entrySet().iterator(), colsToFetch, false);
TabletState state = tm.getTabletState(tservers);

Expand Down

0 comments on commit 90bcb46

Please sign in to comment.