Review: OAK-8827 AWS support for segment-tar by synox · Pull Request #178 · apache/jackrabbit-oak

synox · 2020-02-06T12:47:26Z

Do not merge.

dulceanu

Finished 1st pass through the code.

dulceanu · 2020-02-10T17:00:17Z

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/AwsAppendableFile.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class AwsAppendableFile {


One question related to this class: why not include its functionality as part of AwsJournalFile and get rid of it altogether? To me it doesn't make sense to keep it around.

agreed, removed this file.

dulceanu · 2020-02-10T17:02:26Z

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/AwsArchiveManager.java

+
+    @Override
+    public SegmentArchiveReader open(String archiveName) throws IOException {
+        AwsContext directoryContext = awsContext.withDirectory(archiveName);


AwsContext has two responsibilities: providing the overall context for all AWS operations and also more granular context relative to an archive/directory in S3.. I think it's a bit misleading and makes also some random parts of the code harder to read.

This has been split into a DynamoDBClient class for handling operations on tables and S3Directory class for handling operations within a specific directory in an S3 bucket

dulceanu · 2020-02-10T17:04:53Z

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/AwsContext.java

+            rootDirectory = rootDirectory.substring(1);
+        }
+
+        AWSCredentials credentials = configuration.sessionToken() == null || configuration.sessionToken().isEmpty()


Per Miroslav's comments:

General security reccomendation in AWS, when consuming services (S3 and DynamoDB in this case) is to prefer using temporary security credentials instead of long-term access keys.
https://docs.aws.amazon.com/general/latest/gr/aws-access-keys-best-practices.html#use-roles

Seems that class AwsContext.java is using long-term access keys.
Link below explains how AWS IAM roles can be associated with k8n service account.
https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts.html

Credentials provider that is used currently is fine during development/testing.

It supports both. Long-term access is obtained by using accessKey and secretKey of either the root user or an IAM user.

Temporary security credentials work by using a sessionToken in conjunction with accesKey and secretKey. More details here: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html#instance-metadata-security-credentials

The current implementation does support sessionToken and uses it when specified.

dulceanu · 2020-02-10T17:05:42Z

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/AwsContext.java

+
+        AmazonS3 s3 = AmazonS3ClientBuilder.standard().withCredentials(credentialsProvider).withRegion(region)
+                .withRequestHandlers(handler).build();
+        AmazonDynamoDB ddb = AmazonDynamoDBClientBuilder.standard().withCredentials(credentialsProvider)


Is it ok to re-use the previous hander here? Our request metrics will be skewed b/c frequent calls to update journal...

this matches what we're doing in oak-segment-azure - every journal update is a blob append operation which results in a network call and that gets logged

dulceanu · 2020-02-10T17:30:41Z

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/AwsContext.java

+                .with(TABLE_ATTR_CONTENT, line);
+        try {
+            try {
+                Thread.sleep(1L);


I don't get this part: what's the purpose of the call to Thread.sleep?

dulceanu · 2020-02-10T17:34:13Z

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/AwsPersistence.java

+
+    protected final AwsContext awsContext;
+
+    private final String fileNameSuffix;


I couldn't understand what's this suffix and how it's used...

This is necessary to handle the IT where it spins up multiple stores are created in the setUpCluster method and we use a single bucket for that.. see changes to
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/fixture/SegmentTarFixture.java

UPDATE: changed the IT to use multiple buckets and removed this suffix

dulceanu · 2020-02-10T17:34:33Z

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/AwsPersistence.java

+        this(awsContext, null);
+    }
+
+    public AwsPersistence(AwsContext awsContext, String id) {


The same applies for this id.

same comment as above... in the setUpCluster method multiple stores are setup this suffix is used to identify the entries for journal files for each store in the cluster

UPDATE: the IT has been updated and id and fileNameSuffix have been removed altogether

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/AwsRepositoryLock.java

dulceanu · 2020-02-10T17:36:23Z

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/AwsSegmentStoreService.java

+@Component(configurationPolicy = ConfigurationPolicy.REQUIRE, configurationPid = { Configuration.PID })
+public class AwsSegmentStoreService {
+
+    public static final String DEFAULT_BUCKET_NAME = "oak";


unused constant

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/Configuration.java

synox

Please also update the docs for oak-run: https://github.com/alvarorahul/jackrabbit-oak/blame/alvarorahul/oak-segment-aws/oak-doc/src/site/markdown/nodestore/segment/overview.md#L673

synox · 2020-02-06T14:40:29Z

...-aws/src/test/java/org/apache/jackrabbit/oak/segment/aws/tool/SegmentCopyAzureToTarTest.java

+import org.junit.Before;
+import org.junit.ClassRule;
+
+public class SegmentCopyAzureToTarTest extends SegmentCopyTestBase {


Should be SegmentCopyAwsToTarTest

synox · 2020-02-06T15:04:23Z

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/Configuration.java

+    @AttributeDefinition(
+            name = "Root directory",
+            description = "Names of all the created blobs will be prefixed with this path")
+    String journalTableName() default AwsSegmentStoreService.DEFAULT_JOURNALTABLE_NAME;


Missing definition.

name = "AWS DynamoDB journal table name", description = "Name of table used for storing log entries for journal and gc.")

synox · 2020-02-06T15:04:38Z

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/Configuration.java

+    @AttributeDefinition(
+            name = "Root directory",
+            description = "Names of all the created blobs will be prefixed with this path")
+    String lockTableName() default AwsSegmentStoreService.DEFAULT_LOCKTABLE_NAME;


Missing definition.

name = "AWS DynamoDB lock table name", description = "Name of table used for managing the distributed lock.")

synox · 2020-02-14T14:11:42Z

oak-segment-aws/src/main/java/org/apache/jackrabbit/oak/segment/aws/AwsArchiveManager.java

The Pattern object should be a final static constant of the class.

Project folder oak-segment-aws and POM

Implement ManifestFile

GCJournal file backed by DynamoDB

Journal file implementation for AWS

repository lock implementation using dynamodb-lock-client

SegmentWriteQueue for sending archive entries to AWS

Archive manager and Persistence

Fixture for AWS and integration tests

SegmentCopyCommand capability to copy between AWS and TAR persistence

CompactCommand support for AWS

AwsSegmentStoreService to setup persistence for custom store

…eption

https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingMetadata.html Amazon S3 stores user-defined metadata keys in lowercase.

…pycommand

Track requests, errors and duration for AWS calls

dulceanu requested changes Feb 10, 2020

View reviewed changes

alvarorahul force-pushed the alvarorahul/oak-segment-aws branch from 649eb29 to 0736930 Compare February 14, 2020 07:05

synox commented Feb 14, 2020

View reviewed changes

alvarorahul force-pushed the alvarorahul/oak-segment-aws branch 2 times, most recently from 63e88fd to d1dc253 Compare February 24, 2020 20:59

Alvaro Dias and others added 18 commits February 28, 2020 13:32

OAK-8827 - AWS support for segment-tar

7d9b9f9

Project folder oak-segment-aws and POM

OAK-8827 - AWS support for segment-tar

bccf7f7

Implement ManifestFile

OAK-8827 - AWS support for segment-tar

206f6be

GCJournal file backed by DynamoDB

OAK-8827 - AWS support for segment-tar

5811979

Journal file implementation for AWS

OAK-8827 - AWS support for segment-tar

c1f139b

repository lock implementation using dynamodb-lock-client

OAK-8827 - AWS support for segment-tar

3528d6e

SegmentWriteQueue for sending archive entries to AWS

OAK-8827 - AWS support for segment-tar

a489ea7

Archive manager and Persistence

OAK-8827 - AWS support for segment-tar

69db78a

Fixture for AWS and integration tests

OAK-8827 - AWS support for segment-tar

361334e

SegmentCopyCommand capability to copy between AWS and TAR persistence

OAK-8827 - AWS support for segment-tar

3c81b3c

CompactCommand support for AWS

OAK-8827 - AWS support for segment-tar

9cdb2ed

AwsSegmentStoreService to setup persistence for custom store

not all bytes being read from segment resulting in SegmentNotFoundExc…

acc617a

…eption

Address review comments and update versions of dependencies

eff0b65

Change AWS metadata keys to lowercase

d5fc8d5

https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingMetadata.html Amazon S3 stores user-defined metadata keys in lowercase.

Split AwsContext into DynamoDBClient and S3Directory classes

a966eda

Fix Publish AWS warnings

c194db2

Use timeout value specified in config for the distributed lock

fc92ef9

OAK-8827 - Handle revisions and consume 'last' parameter in segmentco…

ea20ca9

…pycommand

alvarorahul force-pushed the alvarorahul/oak-segment-aws branch from d1dc253 to 84ffb71 Compare February 28, 2020 21:34

OAK-8827 - AWS support for segment-tar

3e7a30c

Track requests, errors and duration for AWS calls

alvarorahul force-pushed the alvarorahul/oak-segment-aws branch from 84ffb71 to 43943e0 Compare February 28, 2020 22:15

Alvaro Dias added 2 commits February 28, 2020 16:17

OAK-8827 - fix for listObjects not following continuation tokens

76403ef

Update version of oak-segment-aws to current iteration

ab1fb13

alvarorahul force-pushed the alvarorahul/oak-segment-aws branch from 43943e0 to ab1fb13 Compare February 29, 2020 00:18

AWS oak-run: class not found: org/apache/commons/lang3/StringUtils

4c16565

Attempt at fixing performance issue when opening AwsSegmentArchiveReader

d0e778b

alvarorahul force-pushed the alvarorahul/oak-segment-aws branch from fc36f71 to d0e778b Compare March 9, 2020 08:26

Update markdown in oak-doc for oak-segment-aws

e6608e1

synox closed this Jun 11, 2021


		protected final AwsContext awsContext;

		private final String fileNameSuffix;

Conversation

synox commented Feb 6, 2020

Uh oh!

dulceanu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvarorahul Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvarorahul Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

synox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alvarorahul Feb 11, 2020 •

edited

Loading

alvarorahul Feb 11, 2020 •

edited

Loading