GCS offload support(3): add configs to support GCS driver #2151

zhaijack · 2018-07-13T03:32:18Z

This is the third part to support Google Cloud Storage offload.
It aims to support GCS related config in ManagedLedgerOffloader. It is based on PR #2065. Please only review the commits after e5b1f7a.

Currently it passed the real test in GCS, by test case ManagedLedgerOffloaderTest#testGcsRealOffload, since lack of GCS mock docker image, will try to use read GCS to do the integration test in later PR.

Master Issue: #2067

…lOffload

…erOffloadServiceAccountKeyFile

zhaijack · 2018-07-23T15:49:30Z

retest this please

sijie · 2018-07-23T21:27:04Z

@ivankelly can you review this?

ivankelly · 2018-07-24T08:49:55Z

conf/broker.conf

@@ -487,6 +487,8 @@ schemaRegistryStorageClassName=org.apache.pulsar.broker.service.schema.Bookkeepe
 ### --- Ledger Offloading --- ###

 # Driver to use to offload old data to long term storage (Possible values: S3, aws-s3, google-cloud-storage)
+# When using gcs, Make sure both Google Cloud Storage and Google Cloud Storage JSON API are enabled for


nit: gcs -> google-cloud-storage, though maybe it would be better to rename the driver gcs, as the options for it are prefixed with gcs.
nit: Make -> make

ivankelly · 2018-07-24T08:56:24Z

conf/broker.conf

@@ -507,6 +509,24 @@ s3ManagedLedgerOffloadMaxBlockSizeInBytes=67108864
 # For Amazon S3 ledger offload, Read buffer size in bytes (1MB by default)
 s3ManagedLedgerOffloadReadBufferSizeInBytes=1048576

+# For Google Cloud Storage ledger offload region


For Google Cloud Storage ledger offload, region where offload bucket is located.

ivankelly · 2018-07-24T09:22:11Z

conf/broker.conf

+# For Google Cloud Storage ledger offload, Bucket to place offloaded ledger into
+gcsManagedLedgerOffloadBucket=
+
+# For Google Cloud Storage ledger offload, Max block size in bytes. (64MB by default, 5MB minimum)


5MB minimum was an AWS imposed constraint. It's probably different for GCS.

Thanks, Yes, It is also 5 MB in GCS.
https://cloud.google.com/storage/docs/json_api/v1/how-tos/multipart-upload
Seems GCS make some alignment with aws s3

ivankelly · 2018-07-24T09:24:02Z

conf/broker.conf

+# For Google Cloud Storage ledger offload, Read buffer size in bytes (1MB by default)
+gcsManagedLedgerOffloadReadBufferSizeInBytes=1048576
+
+# Note: you need first config this to get gcs avliable.


Remove this line. It's redundant.

ivankelly · 2018-07-24T09:35:56Z

conf/broker.conf

+gcsManagedLedgerOffloadReadBufferSizeInBytes=1048576
+
+# Note: you need first config this to get gcs avliable.
+# For Google Cloud Storage credentials of service account key path, usually it is a json file.


For Google Cloud Storage, path to json file containing service account credentials.
For more details, see the "Service Accounts" section of https://support.google.com/googleapi/answer/6158849

ivankelly · 2018-07-24T09:37:21Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

+    private int gcsManagedLedgerOffloadReadBufferSizeInBytes = 1024 * 1024; // 1MB
+
+    // For Google Cloud Storage credentials of service account key path
+    // reference this page for more details of service account key: https://support.google.com/googleapi/answer/6158849


All comments from conf file also apply here, though I'm not sure if this comments are even useful. We should probably just group them all together and point user to broker.conf if they want details, as they will never update the values in this file.

ivankelly · 2018-07-24T09:44:44Z

...ker/src/main/java/org/apache/pulsar/broker/offload/impl/BlobStoreManagedLedgerOffloader.java

+            try {
+                gcsKeyContent = Files.toString(new File(gcsKeyPath), Charset.defaultCharset());
+            } catch (IOException ioe) {
+                log.error("meet exception when read gcs service account key file ");


-> "Cannot read GCS service account credentials file " + gcsKeyPath

ivankelly · 2018-07-24T09:47:13Z

...ker/src/main/java/org/apache/pulsar/broker/offload/impl/BlobStoreManagedLedgerOffloader.java

-        if (credentials != null) {
-            id = credentials.getAWSAccessKeyId();
-            key = credentials.getAWSSecretKey();
+        if (isS3Driver(driver)) {


We shouldn't have AWS/GCS specific logic in this class. Instead, it should be in the factory method, maybe broken out into a utility method called from the factory method. Then the id & key should be passed in as parameters to the constructor of this class.

ivankelly · 2018-07-24T09:48:17Z

...src/test/java/org/apache/pulsar/broker/offload/impl/BlobStoreManagedLedgerOffloaderTest.java

+    public void testGcsNoBucketConfigured() throws Exception {
+        ServiceConfiguration conf = new ServiceConfiguration();
+        conf.setManagedLedgerOffloadDriver("google-cloud-storage");
+        //conf.setGcsManagedLedgerOffloadServiceAccountKeyFile("~/Downloads/project-804d5e6a6f33.json");


Don't commit commented out code.

ivankelly · 2018-07-24T09:49:06Z

...src/test/java/org/apache/pulsar/broker/offload/impl/BlobStoreManagedLedgerOffloaderTest.java

+            Assert.fail("Should have thrown exception");
+        } catch (PulsarServerException pse) {
+            // correct
+            log.error("expect pse", pse);


log.info("Expected pse", pse);

zhaijack · 2018-07-24T16:48:39Z

@ivankelly Thanks for the comments, updated it.

ivankelly

lgtm #shipit

### Motivation s3 mock service doesn't need credential. so we will fail to load credentials on s3 integration tests. that is expected. However there is a regression when introducing GCS support in apache#2151: https://github.com/apache/incubator-pulsar/pull/2151/files#diff-9160ffc9df7afcf42a8335f214393c6dR188 ### Changes Don't throw exception when failed to load aws credentials

* Fix TestS3Offload ### Motivation s3 mock service doesn't need credential. so we will fail to load credentials on s3 integration tests. that is expected. However there is a regression when introducing GCS support in #2151: https://github.com/apache/incubator-pulsar/pull/2151/files#diff-9160ffc9df7afcf42a8335f214393c6dR188 ### Changes * Don't throw exception when failed to load aws credentials * change `log.error` to `log.warn`

This is the 4th part to support Google Cloud Storage offload. It aims to add documentations for GCS. And it is based on PR #2151 Master Issue: #2067

zhaijack force-pushed the gcs_support branch from ec43987 to 2ef1e8d Compare July 13, 2018 04:32

zhaijack mentioned this pull request Jul 13, 2018

GCS offload support(4): add documentations for GCS #2152

Merged

jiazhai added 12 commits July 20, 2018 21:51

change s3client to jclouds-blob-store, pass related unit test

bd54e1f

adobe/s3mock update

d4a4543

rename S3 before filenames

bb6e20e

change following ivan's comments

f672d8e

change following ivan's comments, using GetOptions

97f03bc

fix rebase error

7c315cc

change following ivan's comments

dabbf97

change s3client to jclouds-blob-store, pass related unit test

25be192

tmp change, passed real test in ManagedLedgerOffloaderTest#testGcsRea…

f6d248e

…lOffload

remove test case ManagedLedgerOffloaderTest#testGcsRealOffload

f22cba1

rename gcsManagedLedgerOffloadServiceAccountKeyPath to gcsManagedLedg…

46a57a0

…erOffloadServiceAccountKeyFile

rebase master, fix rebase errors

a7eb9b7

zhaijack force-pushed the gcs_support branch from 780dda4 to a7eb9b7 Compare July 20, 2018 14:32

sijie approved these changes Jul 23, 2018

View reviewed changes

sijie assigned zhaijack Jul 23, 2018

sijie added type/feature The PR added a new feature or issue requested a new feature area/tieredstorage labels Jul 23, 2018

sijie added this to the 2.2.0-incubating milestone Jul 23, 2018

sijie requested a review from ivankelly July 23, 2018 21:29

ivankelly requested changes Jul 24, 2018

View reviewed changes

change following ivan's comments

79a0cbc

zhaijack force-pushed the gcs_support branch from 48f83d1 to 79a0cbc Compare July 24, 2018 16:48

ivankelly approved these changes Jul 24, 2018

View reviewed changes

sijie merged commit de05a9b into apache:master Jul 24, 2018

sijie mentioned this pull request Jul 25, 2018

Fix TestS3Offload #2227

Merged

sijie pushed a commit that referenced this pull request Aug 8, 2018

GCS offload support(4): add documentations for GCS (#2152)

5e1ee37

This is the 4th part to support Google Cloud Storage offload. It aims to add documentations for GCS. And it is based on PR #2151 Master Issue: #2067

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCS offload support(3): add configs to support GCS driver #2151

GCS offload support(3): add configs to support GCS driver #2151

zhaijack commented Jul 13, 2018 •

edited

Loading

zhaijack commented Jul 23, 2018

sijie commented Jul 23, 2018

ivankelly Jul 24, 2018

ivankelly Jul 24, 2018

ivankelly Jul 24, 2018

zhaijack Jul 24, 2018

ivankelly Jul 24, 2018

ivankelly Jul 24, 2018

ivankelly Jul 24, 2018

ivankelly Jul 24, 2018

ivankelly Jul 24, 2018

ivankelly Jul 24, 2018

ivankelly Jul 24, 2018

zhaijack commented Jul 24, 2018

ivankelly left a comment

GCS offload support(3): add configs to support GCS driver #2151

GCS offload support(3): add configs to support GCS driver #2151

Conversation

zhaijack commented Jul 13, 2018 • edited Loading

zhaijack commented Jul 23, 2018

sijie commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaijack commented Jul 24, 2018

ivankelly left a comment

Choose a reason for hiding this comment

zhaijack commented Jul 13, 2018 •

edited

Loading