Skip to content

Commit 9d523d0

Browse files
authored
Merge AwsS3Service and InternalAwsS3Service in a S3Service class (#31580)
The interface and its implementation can be merged into a single class, which is renamed to S3Service like the other S3BlobStore, S3Repository classes.
1 parent 8557bba commit 9d523d0

12 files changed

+64
-199
lines changed

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

-43
This file was deleted.

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

+2-3
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import com.amazonaws.services.s3.model.ObjectListing;
2626
import com.amazonaws.services.s3.model.S3ObjectSummary;
2727
import com.amazonaws.services.s3.model.StorageClass;
28-
2928
import org.elasticsearch.common.blobstore.BlobContainer;
3029
import org.elasticsearch.common.blobstore.BlobPath;
3130
import org.elasticsearch.common.blobstore.BlobStore;
@@ -40,7 +39,7 @@
4039

4140
class S3BlobStore extends AbstractComponent implements BlobStore {
4241

43-
private final AwsS3Service service;
42+
private final S3Service service;
4443

4544
private final String clientName;
4645

@@ -54,7 +53,7 @@ class S3BlobStore extends AbstractComponent implements BlobStore {
5453

5554
private final StorageClass storageClass;
5655

57-
S3BlobStore(Settings settings, AwsS3Service service, String clientName, String bucket, boolean serverSideEncryption,
56+
S3BlobStore(Settings settings, S3Service service, String clientName, String bucket, boolean serverSideEncryption,
5857
ByteSizeValue bufferSize, String cannedACL, String storageClass) {
5958
super(settings);
6059
this.service = service;

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

+9-8
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
package org.elasticsearch.repositories.s3;
2121

2222
import com.amazonaws.auth.BasicAWSCredentials;
23-
2423
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
2524
import org.elasticsearch.common.Strings;
2625
import org.elasticsearch.common.blobstore.BlobPath;
@@ -156,8 +155,10 @@ class S3Repository extends BlobStoreRepository {
156155
/**
157156
* Constructs an s3 backed repository
158157
*/
159-
S3Repository(RepositoryMetaData metadata, Settings settings, NamedXContentRegistry namedXContentRegistry,
160-
AwsS3Service awsService) throws IOException {
158+
S3Repository(final RepositoryMetaData metadata,
159+
final Settings settings,
160+
final NamedXContentRegistry namedXContentRegistry,
161+
final S3Service service) throws IOException {
161162
super(metadata, settings, namedXContentRegistry);
162163

163164
final String bucket = BUCKET_SETTING.get(metadata.settings());
@@ -188,9 +189,9 @@ class S3Repository extends BlobStoreRepository {
188189
// deprecated behavior: override client credentials from the cluster state
189190
// (repository settings)
190191
if (S3ClientSettings.checkDeprecatedCredentials(metadata.settings())) {
191-
overrideCredentialsFromClusterState(awsService);
192+
overrideCredentialsFromClusterState(service);
192193
}
193-
blobStore = new S3BlobStore(settings, awsService, clientName, bucket, serverSideEncryption, bufferSize, cannedACL, storageClass);
194+
blobStore = new S3BlobStore(settings, service, clientName, bucket, serverSideEncryption, bufferSize, cannedACL, storageClass);
194195

195196
final String basePath = BASE_PATH_SETTING.get(metadata.settings());
196197
if (Strings.hasLength(basePath)) {
@@ -220,13 +221,13 @@ protected ByteSizeValue chunkSize() {
220221
return chunkSize;
221222
}
222223

223-
void overrideCredentialsFromClusterState(AwsS3Service awsService) {
224+
void overrideCredentialsFromClusterState(final S3Service s3Service) {
224225
deprecationLogger.deprecated("Using s3 access/secret key from repository settings. Instead "
225226
+ "store these in named clients and the elasticsearch keystore for secure settings.");
226227
final BasicAWSCredentials insecureCredentials = S3ClientSettings.loadDeprecatedCredentials(metadata.settings());
227228
// hack, but that's ok because the whole if branch should be axed
228-
final Map<String, S3ClientSettings> prevSettings = awsService.refreshAndClearCache(S3ClientSettings.load(Settings.EMPTY));
229+
final Map<String, S3ClientSettings> prevSettings = s3Service.refreshAndClearCache(S3ClientSettings.load(Settings.EMPTY));
229230
final Map<String, S3ClientSettings> newSettings = S3ClientSettings.overrideCredentials(prevSettings, insecureCredentials);
230-
awsService.refreshAndClearCache(newSettings);
231+
s3Service.refreshAndClearCache(newSettings);
231232
}
232233
}

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

+25-28
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,6 @@
1919

2020
package org.elasticsearch.repositories.s3;
2121

22-
import java.io.IOException;
23-
import java.security.AccessController;
24-
import java.security.PrivilegedAction;
25-
import java.util.Arrays;
26-
import java.util.Collections;
27-
import java.util.List;
28-
import java.util.Map;
29-
3022
import com.amazonaws.util.json.Jackson;
3123
import org.elasticsearch.SpecialPermission;
3224
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
@@ -39,6 +31,15 @@
3931
import org.elasticsearch.plugins.RepositoryPlugin;
4032
import org.elasticsearch.repositories.Repository;
4133

34+
import java.io.IOException;
35+
import java.security.AccessController;
36+
import java.security.PrivilegedAction;
37+
import java.util.Arrays;
38+
import java.util.Collections;
39+
import java.util.List;
40+
import java.util.Map;
41+
import java.util.Objects;
42+
4243
/**
4344
* A plugin to add a repository type that writes to and from the AWS S3.
4445
*/
@@ -60,33 +61,29 @@ public class S3RepositoryPlugin extends Plugin implements RepositoryPlugin, Relo
6061
});
6162
}
6263

63-
private final AwsS3Service awsS3Service;
64+
private final S3Service service;
6465

65-
public S3RepositoryPlugin(Settings settings) {
66-
this.awsS3Service = getAwsS3Service(settings);
67-
// eagerly load client settings so that secure settings are read
68-
final Map<String, S3ClientSettings> clientsSettings = S3ClientSettings.load(settings);
69-
this.awsS3Service.refreshAndClearCache(clientsSettings);
66+
public S3RepositoryPlugin(final Settings settings) {
67+
this(settings, new S3Service(settings));
7068
}
7169

72-
protected S3RepositoryPlugin(AwsS3Service awsS3Service) {
73-
this.awsS3Service = awsS3Service;
74-
}
75-
76-
// proxy method for testing
77-
protected S3Repository getS3Repository(RepositoryMetaData metadata, Settings settings, NamedXContentRegistry namedXContentRegistry)
78-
throws IOException {
79-
return new S3Repository(metadata, settings, namedXContentRegistry, awsS3Service);
70+
S3RepositoryPlugin(final Settings settings, final S3Service service) {
71+
this.service = Objects.requireNonNull(service, "S3 service must not be null");
72+
// eagerly load client settings so that secure settings are read
73+
final Map<String, S3ClientSettings> clientsSettings = S3ClientSettings.load(settings);
74+
this.service.refreshAndClearCache(clientsSettings);
8075
}
8176

8277
// proxy method for testing
83-
protected AwsS3Service getAwsS3Service(Settings settings) {
84-
return new InternalAwsS3Service(settings);
78+
protected S3Repository createRepository(final RepositoryMetaData metadata,
79+
final Settings settings,
80+
final NamedXContentRegistry registry) throws IOException {
81+
return new S3Repository(metadata, settings, registry, service);
8582
}
8683

8784
@Override
88-
public Map<String, Repository.Factory> getRepositories(Environment env, NamedXContentRegistry namedXContentRegistry) {
89-
return Collections.singletonMap(S3Repository.TYPE, (metadata) -> getS3Repository(metadata, env.settings(), namedXContentRegistry));
85+
public Map<String, Repository.Factory> getRepositories(final Environment env, final NamedXContentRegistry registry) {
86+
return Collections.singletonMap(S3Repository.TYPE, (metadata) -> createRepository(metadata, env.settings(), registry));
9087
}
9188

9289
@Override
@@ -112,11 +109,11 @@ public List<Setting<?>> getSettings() {
112109
public void reload(Settings settings) {
113110
// secure settings should be readable
114111
final Map<String, S3ClientSettings> clientsSettings = S3ClientSettings.load(settings);
115-
awsS3Service.refreshAndClearCache(clientsSettings);
112+
service.refreshAndClearCache(clientsSettings);
116113
}
117114

118115
@Override
119116
public void close() throws IOException {
120-
awsS3Service.close();
117+
service.close();
121118
}
122119
}
+4-5
Original file line numberDiff line numberDiff line change
@@ -28,24 +28,25 @@
2828
import com.amazonaws.internal.StaticCredentialsProvider;
2929
import com.amazonaws.services.s3.AmazonS3;
3030
import com.amazonaws.services.s3.AmazonS3Client;
31-
3231
import org.apache.logging.log4j.Logger;
3332
import org.elasticsearch.common.Strings;
3433
import org.elasticsearch.common.collect.MapBuilder;
3534
import org.elasticsearch.common.component.AbstractComponent;
3635
import org.elasticsearch.common.settings.Settings;
3736

37+
import java.io.Closeable;
3838
import java.io.IOException;
3939
import java.util.Map;
40+
4041
import static java.util.Collections.emptyMap;
4142

4243

43-
class InternalAwsS3Service extends AbstractComponent implements AwsS3Service {
44+
class S3Service extends AbstractComponent implements Closeable {
4445

4546
private volatile Map<String, AmazonS3Reference> clientsCache = emptyMap();
4647
private volatile Map<String, S3ClientSettings> clientsSettings = emptyMap();
4748

48-
InternalAwsS3Service(Settings settings) {
49+
S3Service(Settings settings) {
4950
super(settings);
5051
}
5152

@@ -55,7 +56,6 @@ class InternalAwsS3Service extends AbstractComponent implements AwsS3Service {
5556
* clients are usable until released. On release they will be destroyed instead
5657
* to being returned to the cache.
5758
*/
58-
@Override
5959
public synchronized Map<String, S3ClientSettings> refreshAndClearCache(Map<String, S3ClientSettings> clientsSettings) {
6060
// shutdown all unused clients
6161
// others will shutdown on their respective release
@@ -71,7 +71,6 @@ public synchronized Map<String, S3ClientSettings> refreshAndClearCache(Map<Strin
7171
* Attempts to retrieve a client by name from the cache. If the client does not
7272
* exist it will be created.
7373
*/
74-
@Override
7574
public AmazonS3Reference client(String clientName) {
7675
AmazonS3Reference clientReference = clientsCache.get(clientName);
7776
if ((clientReference != null) && clientReference.tryIncRef()) {

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

+2-78
Original file line numberDiff line numberDiff line change
@@ -65,82 +65,6 @@ public final void wipeAfter() {
6565
cleanRepositoryFiles(basePath);
6666
}
6767

68-
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch-cloud-aws/issues/211")
69-
public void testSimpleWorkflow() {
70-
Client client = client();
71-
Settings.Builder settings = Settings.builder()
72-
.put(S3Repository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(1000, 10000));
73-
74-
// We sometime test getting the base_path from node settings using repositories.s3.base_path
75-
settings.put(S3Repository.BASE_PATH_SETTING.getKey(), basePath);
76-
77-
logger.info("--> creating s3 repository with bucket[{}] and path [{}]", internalCluster().getInstance(Settings.class).get("repositories.s3.bucket"), basePath);
78-
PutRepositoryResponse putRepositoryResponse = client.admin().cluster().preparePutRepository("test-repo")
79-
.setType("s3").setSettings(settings
80-
).get();
81-
assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true));
82-
83-
createIndex("test-idx-1", "test-idx-2", "test-idx-3");
84-
ensureGreen();
85-
86-
logger.info("--> indexing some data");
87-
for (int i = 0; i < 100; i++) {
88-
index("test-idx-1", "doc", Integer.toString(i), "foo", "bar" + i);
89-
index("test-idx-2", "doc", Integer.toString(i), "foo", "baz" + i);
90-
index("test-idx-3", "doc", Integer.toString(i), "foo", "baz" + i);
91-
}
92-
refresh();
93-
assertThat(client.prepareSearch("test-idx-1").setSize(0).get().getHits().getTotalHits(), equalTo(100L));
94-
assertThat(client.prepareSearch("test-idx-2").setSize(0).get().getHits().getTotalHits(), equalTo(100L));
95-
assertThat(client.prepareSearch("test-idx-3").setSize(0).get().getHits().getTotalHits(), equalTo(100L));
96-
97-
logger.info("--> snapshot");
98-
CreateSnapshotResponse createSnapshotResponse = client.admin().cluster().prepareCreateSnapshot("test-repo", "test-snap").setWaitForCompletion(true).setIndices("test-idx-*", "-test-idx-3").get();
99-
assertThat(createSnapshotResponse.getSnapshotInfo().successfulShards(), greaterThan(0));
100-
assertThat(createSnapshotResponse.getSnapshotInfo().successfulShards(), equalTo(createSnapshotResponse.getSnapshotInfo().totalShards()));
101-
102-
assertThat(client.admin().cluster().prepareGetSnapshots("test-repo").setSnapshots("test-snap").get().getSnapshots().get(0).state(), equalTo(SnapshotState.SUCCESS));
103-
104-
logger.info("--> delete some data");
105-
for (int i = 0; i < 50; i++) {
106-
client.prepareDelete("test-idx-1", "doc", Integer.toString(i)).get();
107-
}
108-
for (int i = 50; i < 100; i++) {
109-
client.prepareDelete("test-idx-2", "doc", Integer.toString(i)).get();
110-
}
111-
for (int i = 0; i < 100; i += 2) {
112-
client.prepareDelete("test-idx-3", "doc", Integer.toString(i)).get();
113-
}
114-
refresh();
115-
assertThat(client.prepareSearch("test-idx-1").setSize(0).get().getHits().getTotalHits(), equalTo(50L));
116-
assertThat(client.prepareSearch("test-idx-2").setSize(0).get().getHits().getTotalHits(), equalTo(50L));
117-
assertThat(client.prepareSearch("test-idx-3").setSize(0).get().getHits().getTotalHits(), equalTo(50L));
118-
119-
logger.info("--> close indices");
120-
client.admin().indices().prepareClose("test-idx-1", "test-idx-2").get();
121-
122-
logger.info("--> restore all indices from the snapshot");
123-
RestoreSnapshotResponse restoreSnapshotResponse = client.admin().cluster().prepareRestoreSnapshot("test-repo", "test-snap").setWaitForCompletion(true).execute().actionGet();
124-
assertThat(restoreSnapshotResponse.getRestoreInfo().totalShards(), greaterThan(0));
125-
126-
ensureGreen();
127-
assertThat(client.prepareSearch("test-idx-1").setSize(0).get().getHits().getTotalHits(), equalTo(100L));
128-
assertThat(client.prepareSearch("test-idx-2").setSize(0).get().getHits().getTotalHits(), equalTo(100L));
129-
assertThat(client.prepareSearch("test-idx-3").setSize(0).get().getHits().getTotalHits(), equalTo(50L));
130-
131-
// Test restore after index deletion
132-
logger.info("--> delete indices");
133-
cluster().wipeIndices("test-idx-1", "test-idx-2");
134-
logger.info("--> restore one index after deletion");
135-
restoreSnapshotResponse = client.admin().cluster().prepareRestoreSnapshot("test-repo", "test-snap").setWaitForCompletion(true).setIndices("test-idx-*", "-test-idx-2").execute().actionGet();
136-
assertThat(restoreSnapshotResponse.getRestoreInfo().totalShards(), greaterThan(0));
137-
ensureGreen();
138-
assertThat(client.prepareSearch("test-idx-1").setSize(0).get().getHits().getTotalHits(), equalTo(100L));
139-
ClusterState clusterState = client.admin().cluster().prepareState().get().getState();
140-
assertThat(clusterState.getMetaData().hasIndex("test-idx-1"), equalTo(true));
141-
assertThat(clusterState.getMetaData().hasIndex("test-idx-2"), equalTo(false));
142-
}
143-
14468
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch-cloud-aws/issues/211")
14569
public void testEncryption() {
14670
Client client = client();
@@ -179,7 +103,7 @@ public void testEncryption() {
179103

180104
Settings settings = internalCluster().getInstance(Settings.class);
181105
Settings bucket = settings.getByPrefix("repositories.s3.");
182-
try (AmazonS3Reference s3Client = internalCluster().getInstance(AwsS3Service.class).client("default")) {
106+
try (AmazonS3Reference s3Client = internalCluster().getInstance(S3Service.class).client("default")) {
183107
String bucketName = bucket.get("bucket");
184108
logger.info("--> verify encryption for bucket [{}], prefix [{}]", bucketName, basePath);
185109
List<S3ObjectSummary> summaries = s3Client.client().listObjects(bucketName, basePath).getObjectSummaries();
@@ -442,7 +366,7 @@ public void cleanRepositoryFiles(String basePath) {
442366
// We check that settings has been set in elasticsearch.yml integration test file
443367
// as described in README
444368
assertThat("Your settings in elasticsearch.yml are incorrect. Check README file.", bucketName, notNullValue());
445-
try (AmazonS3Reference s3Client = internalCluster().getInstance(AwsS3Service.class).client("default")) {
369+
try (AmazonS3Reference s3Client = internalCluster().getInstance(S3Service.class).client("default")) {
446370
ObjectListing prevListing = null;
447371
//From http://docs.amazonwebservices.com/AmazonS3/latest/dev/DeletingMultipleObjectsUsingJava.html
448372
//we can do at most 1K objects per delete

0 commit comments

Comments
 (0)