Skip to content

Commit

Permalink
Revert "Add faster scaling composite hash value encoding for remote p…
Browse files Browse the repository at this point in the history
…ath (#13…" (#13244)

This reverts commit 3d1d5e7.
  • Loading branch information
peternied authored Apr 16, 2024
1 parent 5375970 commit 6e0ed65
Show file tree
Hide file tree
Showing 13 changed files with 69 additions and 732 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
import static org.opensearch.index.remote.RemoteStoreEnums.DataCategory.TRANSLOG;
import static org.opensearch.index.remote.RemoteStoreEnums.DataType.DATA;
import static org.opensearch.index.remote.RemoteStoreEnums.DataType.METADATA;
import static org.opensearch.indices.IndicesService.CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING;
import static org.opensearch.indices.IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;
Expand Down Expand Up @@ -229,7 +229,7 @@ public void testRemoteStoreCustomDataOnIndexCreationAndRestore() {
client(clusterManagerNode).admin()
.cluster()
.prepareUpdateSettings()
.setTransientSettings(Settings.builder().put(CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING.getKey(), PathType.FIXED))
.setTransientSettings(Settings.builder().put(CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING.getKey(), PathType.FIXED))
.get();
createRepository(snapshotRepoName, "fs", getRepositorySettings(absolutePath1, true));
Client client = client();
Expand Down Expand Up @@ -260,7 +260,7 @@ public void testRemoteStoreCustomDataOnIndexCreationAndRestore() {
client(clusterManagerNode).admin()
.cluster()
.prepareUpdateSettings()
.setTransientSettings(Settings.builder().put(CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING.getKey(), PathType.HASHED_PREFIX))
.setTransientSettings(Settings.builder().put(CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING.getKey(), PathType.HASHED_PREFIX))
.get();

restoreSnapshotResponse = client.admin()
Expand All @@ -272,13 +272,13 @@ public void testRemoteStoreCustomDataOnIndexCreationAndRestore() {
.get();
assertEquals(RestStatus.ACCEPTED, restoreSnapshotResponse.status());
ensureGreen(restoredIndexName1version2);
validatePathType(restoredIndexName1version2, PathType.HASHED_PREFIX, PathHashAlgorithm.FNV_1A_COMPOSITE_1);
validatePathType(restoredIndexName1version2, PathType.HASHED_PREFIX, PathHashAlgorithm.FNV_1A);

// Create index with cluster setting cluster.remote_store.index.path.type as hashed_prefix.
// Create index with cluster setting cluster.remote_store.index.path.prefix.type as hashed_prefix.
indexSettings = getIndexSettings(1, 0).build();
createIndex(indexName2, indexSettings);
ensureGreen(indexName2);
validatePathType(indexName2, PathType.HASHED_PREFIX, PathHashAlgorithm.FNV_1A_COMPOSITE_1);
validatePathType(indexName2, PathType.HASHED_PREFIX, PathHashAlgorithm.FNV_1A);

// Validating that custom data has not changed for indexes which were created before the cluster setting got updated
validatePathType(indexName1, PathType.FIXED);
Expand All @@ -294,7 +294,7 @@ public void testRemoteStoreCustomDataOnIndexCreationAndRestore() {
client(clusterManagerNode).admin()
.cluster()
.prepareUpdateSettings()
.setTransientSettings(Settings.builder().put(CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING.getKey(), PathType.FIXED))
.setTransientSettings(Settings.builder().put(CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING.getKey(), PathType.FIXED))
.get();

// Close index 2
Expand All @@ -309,7 +309,7 @@ public void testRemoteStoreCustomDataOnIndexCreationAndRestore() {
ensureGreen(indexName2);

// Validating that custom data has not changed for testindex2 which was created before the cluster setting got updated
validatePathType(indexName2, PathType.HASHED_PREFIX, PathHashAlgorithm.FNV_1A_COMPOSITE_1);
validatePathType(indexName2, PathType.HASHED_PREFIX, PathHashAlgorithm.FNV_1A);
}

private void validatePathType(String index, PathType pathType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -713,8 +713,7 @@ public void apply(Settings value, Settings current, Settings previous) {
RemoteStoreNodeService.MIGRATION_DIRECTION_SETTING,
IndicesService.CLUSTER_REMOTE_INDEX_RESTRICT_ASYNC_DURABILITY_SETTING,
IndicesService.CLUSTER_INDEX_RESTRICT_REPLICATION_TYPE_SETTING,
IndicesService.CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING,
IndicesService.CLUSTER_REMOTE_STORE_PATH_HASH_ALGORITHM_SETTING,
IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING,

// Admission Control Settings
AdmissionControlSettings.ADMISSION_CONTROL_TRANSPORT_LAYER_MODE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
import static java.util.Collections.unmodifiableMap;
import static org.opensearch.index.remote.RemoteStoreEnums.DataType.DATA;
import static org.opensearch.index.remote.RemoteStoreEnums.DataType.METADATA;
import static org.opensearch.index.remote.RemoteStoreUtils.longToCompositeBase64AndBinaryEncoding;
import static org.opensearch.index.remote.RemoteStoreUtils.longToUrlBase64;

/**
* This class contains the different enums related to remote store like data categories and types, path types
Expand Down Expand Up @@ -218,26 +216,13 @@ public static PathType parseString(String pathType) {
@PublicApi(since = "2.14.0")
public enum PathHashAlgorithm {

FNV_1A_BASE64(0) {
FNV_1A(0) {
@Override
String hash(PathInput pathInput) {
String input = pathInput.indexUUID() + pathInput.shardId() + pathInput.dataCategory().getName() + pathInput.dataType()
.getName();
long hash = FNV1a.hash64(input);
return longToUrlBase64(hash);
}
},
/**
* This hash algorithm will generate a hash value which will use 1st 6 bits to create bas64 character and next 14
* bits to create binary string.
*/
FNV_1A_COMPOSITE_1(1) {
@Override
String hash(PathInput pathInput) {
String input = pathInput.indexUUID() + pathInput.shardId() + pathInput.dataCategory().getName() + pathInput.dataType()
.getName();
long hash = FNV1a.hash64(input);
return longToCompositeBase64AndBinaryEncoding(hash, 20);
return RemoteStoreUtils.longToUrlBase64(hash);
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,12 @@ public class RemoteStorePathStrategyResolver {

private volatile PathType type;

private volatile PathHashAlgorithm hashAlgorithm;

private final Supplier<Version> minNodeVersionSupplier;

public RemoteStorePathStrategyResolver(ClusterSettings clusterSettings, Supplier<Version> minNodeVersionSupplier) {
this.minNodeVersionSupplier = minNodeVersionSupplier;
type = clusterSettings.get(IndicesService.CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING);
hashAlgorithm = clusterSettings.get(IndicesService.CLUSTER_REMOTE_STORE_PATH_HASH_ALGORITHM_SETTING);
clusterSettings.addSettingsUpdateConsumer(IndicesService.CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING, this::setType);
clusterSettings.addSettingsUpdateConsumer(IndicesService.CLUSTER_REMOTE_STORE_PATH_HASH_ALGORITHM_SETTING, this::setHashAlgorithm);
type = clusterSettings.get(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING);
clusterSettings.addSettingsUpdateConsumer(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING, this::setType);
}

public RemoteStorePathStrategy get() {
Expand All @@ -43,15 +39,11 @@ public RemoteStorePathStrategy get() {
// Min node version check ensures that we are enabling the new prefix type only when all the nodes understand it.
pathType = Version.CURRENT.compareTo(minNodeVersionSupplier.get()) <= 0 ? type : PathType.FIXED;
// If the path type is fixed, hash algorithm is not applicable.
pathHashAlgorithm = pathType == PathType.FIXED ? null : hashAlgorithm;
pathHashAlgorithm = pathType == PathType.FIXED ? null : PathHashAlgorithm.FNV_1A;
return new RemoteStorePathStrategy(pathType, pathHashAlgorithm);
}

private void setType(PathType type) {
this.type = type;
}

private void setHashAlgorithm(PathHashAlgorithm hashAlgorithm) {
this.hashAlgorithm = hashAlgorithm;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import java.util.Base64;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.function.Function;

Expand All @@ -27,16 +26,10 @@
public class RemoteStoreUtils {
public static final int LONG_MAX_LENGTH = String.valueOf(Long.MAX_VALUE).length();

/**
* URL safe base 64 character set. This must not be changed as this is used in deriving the base64 equivalent of binary.
*/
static final char[] URL_BASE64_CHARSET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_".toCharArray();

/**
* This method subtracts given numbers from Long.MAX_VALUE and returns a string representation of the result.
* The resultant string is guaranteed to be of the same length that of Long.MAX_VALUE. If shorter, we add left padding
* of 0s to the string.
*
* @param num number to get the inverted long string for
* @return String value of Long.MAX_VALUE - num
*/
Expand All @@ -53,7 +46,6 @@ public static String invertLong(long num) {

/**
* This method converts the given string into long and subtracts it from Long.MAX_VALUE
*
* @param str long in string format to be inverted
* @return long value of the invert result
*/
Expand All @@ -67,7 +59,6 @@ public static long invertLong(String str) {

/**
* Extracts the segment name from the provided segment file name
*
* @param filename Segment file name to parse
* @return Name of the segment that the segment file belongs to
*/
Expand All @@ -88,9 +79,10 @@ public static String getSegmentName(String filename) {
}

/**
*
* @param mdFiles List of segment/translog metadata files
* @param fn Function to extract PrimaryTerm_Generation and Node Id from metadata file name .
* fn returns null if node id is not part of the file name
* @param fn Function to extract PrimaryTerm_Generation and Node Id from metadata file name .
* fn returns null if node id is not part of the file name
*/
public static void verifyNoMultipleWriters(List<String> mdFiles, Function<String, Tuple<String, String>> fn) {
Map<String, String> nodesByPrimaryTermAndGen = new HashMap<>();
Expand Down Expand Up @@ -124,26 +116,4 @@ static String longToUrlBase64(long value) {
String base64Str = Base64.getUrlEncoder().encodeToString(hashBytes);
return base64Str.substring(0, base64Str.length() - 1);
}

static long urlBase64ToLong(String base64Str) {
byte[] hashBytes = Base64.getUrlDecoder().decode(base64Str);
return ByteBuffer.wrap(hashBytes).getLong();
}

/**
* Converts an input hash which occupies 64 bits of memory into a composite encoded string. The string will have 2 parts -
* 1. Base 64 string and 2. Binary String. We will use the first 6 bits for creating the base 64 string.
* For the second part, the rest of the bits (of length {@code len}-6) will be used as is in string form.
*/
static String longToCompositeBase64AndBinaryEncoding(long value, int len) {
if (len < 7 || len > 64) {
throw new IllegalArgumentException("In longToCompositeBase64AndBinaryEncoding, len must be between 7 and 64 (both inclusive)");
}
String binaryEncoding = String.format(Locale.ROOT, "%64s", Long.toBinaryString(value)).replace(' ', '0');
String base64Part = binaryEncoding.substring(0, 6);
String binaryPart = binaryEncoding.substring(6, len);
int base64DecimalValue = Integer.valueOf(base64Part, 2);
assert base64DecimalValue >= 0 && base64DecimalValue < 64;
return URL_BASE64_CHARSET[base64DecimalValue] + binaryPart;
}
}
20 changes: 3 additions & 17 deletions server/src/main/java/org/opensearch/indices/IndicesService.java
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@
import org.opensearch.index.query.QueryRewriteContext;
import org.opensearch.index.recovery.RecoveryStats;
import org.opensearch.index.refresh.RefreshStats;
import org.opensearch.index.remote.RemoteStoreEnums.PathHashAlgorithm;
import org.opensearch.index.remote.RemoteStoreEnums.PathType;
import org.opensearch.index.remote.RemoteStoreStatsTrackerFactory;
import org.opensearch.index.search.stats.SearchStats;
Expand Down Expand Up @@ -308,30 +307,17 @@ public class IndicesService extends AbstractLifecycleComponent
);

/**
* This setting is used to set the remote store blob store path type strategy. This setting is effective only for
* This setting is used to set the remote store blob store path prefix strategy. This setting is effective only for
* remote store enabled cluster.
*/
public static final Setting<PathType> CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING = new Setting<>(
"cluster.remote_store.index.path.type",
public static final Setting<PathType> CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING = new Setting<>(
"cluster.remote_store.index.path.prefix.type",
PathType.FIXED.toString(),
PathType::parseString,
Property.NodeScope,
Property.Dynamic
);

/**
* This setting is used to set the remote store blob store path hash algorithm strategy. This setting is effective only for
* remote store enabled cluster. This setting will come to effect if the {@link #CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING}
* is either {@code HASHED_PREFIX} or {@code HASHED_INFIX}.
*/
public static final Setting<PathHashAlgorithm> CLUSTER_REMOTE_STORE_PATH_HASH_ALGORITHM_SETTING = new Setting<>(
"cluster.remote_store.index.path.hash_algorithm",
PathHashAlgorithm.FNV_1A_COMPOSITE_1.toString(),
PathHashAlgorithm::parseString,
Property.NodeScope,
Property.Dynamic
);

/**
* The node's settings.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1711,7 +1711,7 @@ public void testRemoteCustomData() {
validateRemoteCustomData(
indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY),
PathHashAlgorithm.NAME,
PathHashAlgorithm.FNV_1A_COMPOSITE_1.name()
PathHashAlgorithm.FNV_1A.name()
);
}

Expand All @@ -1720,7 +1720,7 @@ private IndexMetadata testRemoteCustomData(boolean remoteStoreEnabled, PathType
if (remoteStoreEnabled) {
settingsBuilder.put(NODE_ATTRIBUTES.getKey() + REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY, "test");
}
settingsBuilder.put(IndicesService.CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING.getKey(), pathType.toString());
settingsBuilder.put(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING.getKey(), pathType.toString());
Settings settings = settingsBuilder.build();

ClusterService clusterService = mock(ClusterService.class);
Expand Down
Loading

0 comments on commit 6e0ed65

Please sign in to comment.