Skip to content

Commit 8f2a85b

Browse files
original-brownbearoriginal-brownbear
committed
CR: remove redundant reloading of settings and use in cache key
1 parent 3f92d32 commit 8f2a85b

File tree

6 files changed

+23
-51
lines changed

6 files changed

+23
-51
lines changed

‎plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

+4-6
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,6 @@
3030
import org.elasticsearch.common.blobstore.BlobPath;
3131
import org.elasticsearch.common.blobstore.BlobStore;
3232
import org.elasticsearch.common.blobstore.BlobStoreException;
33-
import org.elasticsearch.common.collect.Tuple;
34-
import org.elasticsearch.common.settings.Settings;
3533
import org.elasticsearch.common.unit.ByteSizeValue;
3634

3735
import java.io.IOException;
@@ -52,18 +50,18 @@ class S3BlobStore implements BlobStore {
5250

5351
private final StorageClass storageClass;
5452

55-
private final Tuple<RepositoryMetaData, Settings> settingsKey;
53+
private final RepositoryMetaData repositoryMetaData;
5654

5755
S3BlobStore(S3Service service, String bucket, boolean serverSideEncryption,
5856
ByteSizeValue bufferSize, String cannedACL, String storageClass,
59-
RepositoryMetaData repositoryMetaData, Settings settings) {
57+
RepositoryMetaData repositoryMetaData) {
6058
this.service = service;
6159
this.bucket = bucket;
6260
this.serverSideEncryption = serverSideEncryption;
6361
this.bufferSize = bufferSize;
6462
this.cannedACL = initCannedACL(cannedACL);
6563
this.storageClass = initStorageClass(storageClass);
66-
settingsKey = new Tuple<>(repositoryMetaData, settings);
64+
this.repositoryMetaData = repositoryMetaData;
6765
}
6866

6967
@Override
@@ -72,7 +70,7 @@ public String toString() {
7270
}
7371

7472
public AmazonS3Reference clientReference() {
75-
return service.client(settingsKey);
73+
return service.client(repositoryMetaData);
7674
}
7775

7876
public String bucket() {

‎plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java

+3-19
Original file line numberDiff line numberDiff line change
@@ -140,68 +140,52 @@ private S3ClientSettings(S3BasicCredentials credentials, String endpoint, Protoc
140140
}
141141

142142
/**
143-
* Overrides the settings in this instance with settings found in repository metadata and
144-
* settings from the clusterstate.
143+
* Overrides the settings in this instance with settings found in repository metadata.
144+
*
145145
* @param metadata RepositoryMetaData
146-
* @param settings Cluster Settings
147146
* @return S3ClientSettings
148147
*/
149-
S3ClientSettings refine(RepositoryMetaData metadata, Settings settings) {
148+
S3ClientSettings refine(RepositoryMetaData metadata) {
150149
final Settings repoSettings = metadata.settings();
151-
final String clientName = S3Repository.CLIENT_NAME.get(repoSettings);
152150
final String newEndpoint;
153151
if (ENDPOINT_SETTING.exists(repoSettings)) {
154152
newEndpoint = ENDPOINT_SETTING.get(repoSettings);
155-
} else if (hasConfigValue(settings, clientName, ENDPOINT_SETTING)) {
156-
newEndpoint = getConfigValue(settings, clientName, ENDPOINT_SETTING);
157153
} else {
158154
newEndpoint = endpoint;
159155
}
160156
final Protocol newProtocol;
161157
if (PROTOCOL_SETTING.exists(repoSettings)) {
162158
newProtocol = PROTOCOL_SETTING.get(repoSettings);
163-
} else if (hasConfigValue(settings, clientName, PROTOCOL_SETTING)) {
164-
newProtocol = getConfigValue(settings, clientName, PROTOCOL_SETTING);
165159
} else {
166160
newProtocol = protocol;
167161
}
168162
final String newProxyHost;
169163
if (PROXY_HOST_SETTING.exists(repoSettings)) {
170164
newProxyHost = PROXY_HOST_SETTING.get(repoSettings);
171-
} else if (hasConfigValue(settings, clientName, PROXY_HOST_SETTING)) {
172-
newProxyHost = getConfigValue(settings, clientName, PROXY_HOST_SETTING);
173165
} else {
174166
newProxyHost = proxyHost;
175167
}
176168
final int newProxyPort;
177169
if (PROXY_PORT_SETTING.exists(repoSettings)) {
178170
newProxyPort = PROXY_PORT_SETTING.get(repoSettings);
179-
} else if (hasConfigValue(settings, clientName, PROXY_PORT_SETTING)) {
180-
newProxyPort = getConfigValue(settings, clientName, PROXY_PORT_SETTING);
181171
} else {
182172
newProxyPort = proxyPort;
183173
}
184174
final int newReadTimeoutMillis;
185175
if (READ_TIMEOUT_SETTING.exists(repoSettings)) {
186176
newReadTimeoutMillis = Math.toIntExact(READ_TIMEOUT_SETTING.get(repoSettings).millis());
187-
} else if (hasConfigValue(settings, clientName, READ_TIMEOUT_SETTING)) {
188-
newReadTimeoutMillis = Math.toIntExact(getConfigValue(settings, clientName, READ_TIMEOUT_SETTING).millis());
189177
} else {
190178
newReadTimeoutMillis = readTimeoutMillis;
191179
}
192180
final int newMaxRetries;
193181
if (MAX_RETRIES_SETTING.exists(repoSettings)) {
194182
newMaxRetries = MAX_RETRIES_SETTING.get(repoSettings);
195-
} else if (hasConfigValue(settings, clientName, MAX_RETRIES_SETTING)) {
196-
newMaxRetries = getConfigValue(settings, clientName, MAX_RETRIES_SETTING);
197183
} else {
198184
newMaxRetries = maxRetries;
199185
}
200186
final boolean newThrottleRetries;
201187
if (USE_THROTTLE_RETRIES_SETTING.exists(repoSettings)) {
202188
newThrottleRetries = USE_THROTTLE_RETRIES_SETTING.get(repoSettings);
203-
} else if (hasConfigValue(settings, clientName, USE_THROTTLE_RETRIES_SETTING)) {
204-
newThrottleRetries = getConfigValue(settings, clientName, USE_THROTTLE_RETRIES_SETTING);
205189
} else {
206190
newThrottleRetries = throttleRetries;
207191
}

‎plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java

+1-5
Original file line numberDiff line numberDiff line change
@@ -147,8 +147,6 @@ class S3Repository extends BlobStoreRepository {
147147
*/
148148
static final Setting<String> BASE_PATH_SETTING = Setting.simpleString("base_path");
149149

150-
private final Settings settings;
151-
152150
private final S3Service service;
153151

154152
private final String bucket;
@@ -177,7 +175,6 @@ class S3Repository extends BlobStoreRepository {
177175
final NamedXContentRegistry namedXContentRegistry,
178176
final S3Service service) {
179177
super(metadata, settings, namedXContentRegistry);
180-
this.settings = settings;
181178
this.service = service;
182179

183180
this.repositoryMetaData = metadata;
@@ -228,8 +225,7 @@ class S3Repository extends BlobStoreRepository {
228225

229226
@Override
230227
protected S3BlobStore createBlobStore() {
231-
return new S3BlobStore(service, bucket, serverSideEncryption, bufferSize, cannedACL, storageClass,
232-
repositoryMetaData, settings);
228+
return new S3BlobStore(service, bucket, serverSideEncryption, bufferSize, cannedACL, storageClass, repositoryMetaData);
233229
}
234230

235231
// only use for testing

‎plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java

+12-16
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@
3535
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
3636
import org.elasticsearch.common.Strings;
3737
import org.elasticsearch.common.collect.MapBuilder;
38-
import org.elasticsearch.common.collect.Tuple;
3938
import org.elasticsearch.common.settings.Settings;
4039

4140
import java.io.Closeable;
@@ -60,7 +59,7 @@ class S3Service implements Closeable {
6059
* Client settings derived from those in {@link #staticClientSettings} by combining them with settings
6160
* in the {@link RepositoryMetaData} and {@link Settings} from the clusterstate.
6261
*/
63-
private volatile Map<S3ClientSettings, Map<Tuple<RepositoryMetaData, Settings>, S3ClientSettings>> derivedClientSettings = emptyMap();
62+
private volatile Map<S3ClientSettings, Map<RepositoryMetaData, S3ClientSettings>> derivedClientSettings = emptyMap();
6463

6564
/**
6665
* Refreshes the settings for the AmazonS3 clients and clears the cache of
@@ -82,8 +81,8 @@ public synchronized void refreshAndClearCache(Map<String, S3ClientSettings> clie
8281
* Attempts to retrieve a client by its repository metadata and settings from the cache.
8382
* If the client does not exist it will be created.
8483
*/
85-
public AmazonS3Reference client(Tuple<RepositoryMetaData, Settings> settings) {
86-
final S3ClientSettings clientSettings = settings(settings);
84+
public AmazonS3Reference client(RepositoryMetaData repositoryMetaData) {
85+
final S3ClientSettings clientSettings = settings(repositoryMetaData);
8786
{
8887
final AmazonS3Reference clientReference = clientsCache.get(clientSettings);
8988
if (clientReference != null && clientReference.tryIncRef()) {
@@ -103,34 +102,31 @@ public AmazonS3Reference client(Tuple<RepositoryMetaData, Settings> settings) {
103102
}
104103

105104
/**
106-
* Either fetches {@link S3ClientSettings} for a given combination of {@link RepositoryMetaData} and {@link Settings}
107-
* from cached settings or creates them by overriding static client settings from {@link #staticClientSettings}
108-
* with settings found in repository metadata and the settings.
109-
* @param coordinates Tuple of {@link RepositoryMetaData} and {@link Settings}
105+
* Either fetches {@link S3ClientSettings} for a givne {@link RepositoryMetaData} from cached settings or creates them
106+
* by overriding static client settings from {@link #staticClientSettings} with settings found in the repository metadata.
107+
* @param repositoryMetaData Repository Metadata
110108
* @return S3ClientSettings
111109
*/
112-
private S3ClientSettings settings(Tuple<RepositoryMetaData, Settings> coordinates) {
113-
final RepositoryMetaData repositoryMetaData = coordinates.v1();
110+
private S3ClientSettings settings(RepositoryMetaData repositoryMetaData) {
114111
final String clientName = S3Repository.CLIENT_NAME.get(repositoryMetaData.settings());
115112
final S3ClientSettings staticSettings = staticClientSettings.get(clientName);
116113
if (staticSettings != null) {
117114
{
118-
final S3ClientSettings existing = derivedClientSettings.getOrDefault(staticSettings, emptyMap()).get(coordinates);
115+
final S3ClientSettings existing = derivedClientSettings.getOrDefault(staticSettings, emptyMap()).get(repositoryMetaData);
119116
if (existing != null) {
120117
return existing;
121118
}
122119
}
123-
final Settings settings = coordinates.v2();
124120
synchronized (this) {
125-
final Map<Tuple<RepositoryMetaData, Settings>, S3ClientSettings> derivedSettings =
121+
final Map<RepositoryMetaData, S3ClientSettings> derivedSettings =
126122
derivedClientSettings.getOrDefault(staticSettings, emptyMap());
127-
final S3ClientSettings existing = derivedSettings.get(coordinates);
123+
final S3ClientSettings existing = derivedSettings.get(repositoryMetaData);
128124
if (existing != null) {
129125
return existing;
130126
}
131-
final S3ClientSettings newSettings = staticSettings.refine(repositoryMetaData, settings);
127+
final S3ClientSettings newSettings = staticSettings.refine(repositoryMetaData);
132128
derivedClientSettings = MapBuilder.newMapBuilder(derivedClientSettings).put(
133-
staticSettings, MapBuilder.newMapBuilder(derivedSettings).put(coordinates, newSettings).immutableMap()
129+
staticSettings, MapBuilder.newMapBuilder(derivedSettings).put(repositoryMetaData, newSettings).immutableMap()
134130
).immutableMap();
135131
return newSettings;
136132
}

‎plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobStoreTests.java

+2-3
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
2626
import org.elasticsearch.common.blobstore.BlobStore;
2727
import org.elasticsearch.common.blobstore.BlobStoreException;
28-
import org.elasticsearch.common.collect.Tuple;
2928
import org.elasticsearch.common.settings.Settings;
3029
import org.elasticsearch.common.unit.ByteSizeUnit;
3130
import org.elasticsearch.common.unit.ByteSizeValue;
@@ -120,11 +119,11 @@ public static S3BlobStore randomMockS3BlobStore() {
120119
final AmazonS3 client = new MockAmazonS3(new ConcurrentHashMap<>(), bucket, serverSideEncryption, cannedACL, storageClass);
121120
final S3Service service = new S3Service() {
122121
@Override
123-
public synchronized AmazonS3Reference client(Tuple<RepositoryMetaData, Settings> settings) {
122+
public synchronized AmazonS3Reference client(RepositoryMetaData repositoryMetaData) {
124123
return new AmazonS3Reference(client);
125124
}
126125
};
127126
return new S3BlobStore(service, bucket, serverSideEncryption, bufferSize, cannedACL, storageClass,
128-
new RepositoryMetaData(bucket, "s3", Settings.EMPTY), Settings.EMPTY);
127+
new RepositoryMetaData(bucket, "s3", Settings.EMPTY));
129128
}
130129
}

‎plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121

2222
import com.amazonaws.services.s3.AbstractAmazonS3;
2323
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
24-
import org.elasticsearch.common.collect.Tuple;
2524
import org.elasticsearch.common.settings.Settings;
2625
import org.elasticsearch.common.unit.ByteSizeUnit;
2726
import org.elasticsearch.common.unit.ByteSizeValue;
@@ -49,7 +48,7 @@ public void shutdown() {
4948

5049
private static class DummyS3Service extends S3Service {
5150
@Override
52-
public AmazonS3Reference client(Tuple<RepositoryMetaData, Settings> settings) {
51+
public AmazonS3Reference client(RepositoryMetaData repositoryMetaData) {
5352
return new AmazonS3Reference(new DummyS3Client());
5453
}
5554

0 commit comments

Comments
 (0)