GCS offload support(2): replace `s3client` api with `jclouds` related api #2065

zhaijack · 2018-07-02T16:23:20Z

This is the second part to support Google Cloud Storage offload.
It aims to replace "s3 client" api with "jclouds" api, and make sure unit test and integration test passed.
There will be a following change to add Google Cloud Storage support and related test.

change:
replace s3client api with jclouds related api in S3ManagedLedgerOffloader

Master Issue: #2067

sijie · 2018-07-02T17:27:58Z

@zhaijack I have merged the renames in #2064 . you can rebase to latest master now.

zhaijack · 2018-07-04T07:55:37Z

retest this please

error:
org.apache.pulsar.tests.integration.TestS3Offload.configureAndStartBrokers

07:41:58.384 [main] INFO  org.apache.pulsar.tests.PulsarClusterUtils - Connecting to zookeeper 172.18.0.4
~~~~~~~~~ SKIPPED -- [TestClass name=class org.apache.pulsar.tests.integration.TestS3Offload].null([])-------~~~~~~~~~ SKIPPED -- [TestClass name=class org.apache.pulsar.tests.integration.TestS3Offload].null([])-------07:43:06.995 [pool-2-thread-14] ERROR org.apache.pulsar.tests.DockerUtils - Error reading dir from container configuration-store_25b4cf62-be6d-4fed-95ef-1f05974eec0f
com.github.dockerjava.api.exception.NotFoundException: {"message":"lstat /var/lib/docker/aufs/mnt/aed05eca1add853b786134edec4d7a58eed32ca2aa47b1a6c1e8c0b3ed51baa9/pulsar/data/zookeeper: no such file or directory"}

	at com.github.dockerjava.jaxrs.filter.ResponseStatusExceptionFilter.filter(ResponseStatusExceptionFilter.java:47) ~[docker-java-3.0.14.jar:?]
	at org.glassfish.jersey.client.ClientFilteringStages$ResponseFilterStage.apply(ClientFilteringStages.java:140) ~[jersey-client-2.25.jar:?]
	at org.glassfish.jersey.client.ClientFilteringStages$ResponseFilterStage.apply(ClientFilteringStages.java:128) ~[jersey-client-2.25.jar:?]
	at org.glassfish.jersey.process.internal.Stages.process(Stages.java:171) ~[jersey-common-2.25.jar:?]

zhaijack · 2018-07-05T23:13:55Z

Seems integration test still have some issue, change back to Work In Progress

zhaijack · 2018-07-11T10:07:47Z

retest this please
for ut:
org.apache.pulsar.broker.service.ReplicatorTest.setup
org.apache.pulsar.broker.service.v1.V1_ReplicatorTest.setup
org.apache.pulsar.broker.service.BrokerBkEnsemblesTests.testSkipCorruptDataLedger
org.apache.pulsar.client.impl.SequenceIdWithErrorTest.testCheckSequenceId

ivankelly · 2018-07-13T17:09:32Z

jclouds-shaded/pom.xml

+                  <include>org.apache.jclouds.api:*</include>
+                  <include>org.apache.jclouds.common:*</include>
+                  <include>org.apache.jclouds.provider:*</include>
+                  <include>com.google.inject.extensions:guice-assistedinject</include>


These are new dependencies, which need to be accounted for in the LICENSE/NOTICE.

ivankelly · 2018-07-13T17:10:03Z

pom.xml

@@ -101,6 +101,8 @@ flexible messaging model and an intuitive client API.</description>
    <module>tests</module>
    <module>pulsar-log4j2-appender</module>
    <module>protobuf-shaded</module>
+    <!-- jclouds shaded for gson conflict: https://issues.apache.org/jira/browse/JCLOUDS-1166 -->


we're shading so much crap. there must be a better way.

Thanks, @ivankelly, Opened issue #2164 for this. Once it is done we could remove this shading.

ivankelly · 2018-07-13T17:11:47Z

pulsar-broker/pom.xml

@@ -268,6 +268,12 @@
      <version>${project.version}</version>
      <scope>test</scope>
    </dependency>


you need to remove the aws s3 dependency too.

oh, Seems as you commented, the AWSCredentials related things could not get out.

We shouldn't need the full s3 dependency now, just whatever AWSCredentials is in.

ivankelly · 2018-07-13T17:18:54Z

jclouds-shaded/pom.xml

+    <dependency>
+      <groupId>org.apache.jclouds</groupId>
+      <artifactId>jclouds-allblobstore</artifactId>
+      <version>2.2.0-SNAPSHOT</version>


Thanks, 2.2.1 has some issue related with gcs multi-part upload, so we use the latest. This is tracked in #2164

Ok, it'll have to be updated before an actual release.

ivankelly · 2018-07-13T17:21:57Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/offload/impl/BackedInputStreamImpl.java

-                                   VersionCheck versionCheck,
-                                   long objectLen, int bufferSize) {
-        this.s3client = s3client;
+    public BackedInputStreamImpl(BlobStore blobStore, String bucket, String key,


Rename BlobStoreBackedInputStreamImpl

ivankelly · 2018-07-13T17:28:44Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/offload/impl/ManagedLedgerOffloader.java

@@ -123,6 +191,10 @@ static String indexBlockOffloadKey(long ledgerId, UUID uuid) {
        return String.format("%s-ledger-%d-index", uuid.toString(), ledgerId);
    }

+    public boolean createBucket() {


Seems unused.

OK, will remove it. If needed, we could add it in the future.

ivankelly · 2018-07-13T17:29:42Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/offload/impl/ManagedLedgerOffloader.java

-                        etags.add(uploadRes.getPartETag());
+                        Payload partPayload = Payloads.newInputStreamPayload(blockStream);
+                        partPayload.getContentMetadata().setContentLength((long)blockSize);
+                        partPayload.getContentMetadata().setContentType("text/plain");


It's not text/plain. It's application/octet-stream if anything

ivankelly · 2018-07-13T17:31:15Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/offload/impl/ManagedLedgerOffloader.java

+                addVersionInfo(blobBuilder);
+                Payload indexPayload = Payloads.newInputStreamPayload(indexStream);
+                indexPayload.getContentMetadata().setContentLength((long)indexStream.getStreamSize());
+                indexPayload.getContentMetadata().setContentType("text/plain");


application/octet-stream

ivankelly · 2018-07-13T17:32:38Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/offload/impl/ManagedLedgerOffloader.java

+                blobStore.removeBlob(bucket, dataBlockOffloadKey(ledgerId, uid));
+                blobStore.removeBlob(bucket, indexBlockOffloadKey(ledgerId, uid));
+                // adobe/s3mock not support removeBlobs well.
+                /*blobStore.removeBlobs(bucket,


Since we are shipping out own s3mock, we should fix it.
And I don't understand how it doesn't support removeBlobs if deleteObjects worked fine.

Thanks, will changed it back to have a try.

ivankelly · 2018-07-13T17:35:43Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/offload/BlobStoreTestBase.java

+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+
+public class BlobStoreTestBase {


S3TestBase and the mock can be deleted now, no?

S3TestBase had a feature to allow the test to use real AWS if a system property was set. It would be good to have something similar here to validate that the code is ok.

Thanks, would like to do it later. opened issue #2165 to track this work.

zhaijack · 2018-07-15T06:43:13Z

@ivankelly Thanks for the comments, updated this PR.

ivankelly · 2018-07-16T19:23:49Z

...oker/src/main/java/org/apache/pulsar/broker/offload/impl/BlobStoreBackedInputStreamImpl.java

+            try {
+                Blob blob = blobStore.getBlob(bucket, key);
+                versionCheck.check(key, blob);
+                PayloadSlicer slicer = new BasePayloadSlicer();


Slicer isnt what you need here. getBlob can take a GetOptions parameter. You can set the range in that.

zhaijack · 2018-07-17T06:14:57Z

retest this please

ivankelly · 2018-07-17T10:55:03Z

jclouds-shaded/pom.xml

+    <dependency>
+      <groupId>org.apache.jclouds</groupId>
+      <artifactId>jclouds-allblobstore</artifactId>
+      <version>2.2.0-SNAPSHOT</version>


Ok, it'll have to be updated before an actual release.

ivankelly · 2018-07-17T11:02:18Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/offload/impl/BackedReadHandleImpl.java

 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

-public class S3BackedReadHandleImpl implements ReadHandle {
-    private static final Logger log = LoggerFactory.getLogger(S3BackedReadHandleImpl.class);
+public class BackedReadHandleImpl implements ReadHandle {


This should be BlobStoreBackedReadHandleImpl. Basically, anywhere you removed S3 we should have BlobStore.

Thanks, will change it, if you insist on this.

ivankelly · 2018-07-17T11:06:36Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/offload/impl/ManagedLedgerOffloader.java

+            log.error("Exception when get credentials for s3 ", e);
+        }
+
+        String id = "accesskey";


default these to ""

Thanks. will keep this default value. empty string will meet error in jclouds.

ivankelly · 2018-07-17T11:07:34Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/offload/impl/ManagedLedgerOffloader.java

 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

-public class S3ManagedLedgerOffloader implements LedgerOffloader {
-    private static final Logger log = LoggerFactory.getLogger(S3ManagedLedgerOffloader.class);
+public class ManagedLedgerOffloader implements LedgerOffloader {


rename to BlobStoreManagedLedgerOffloader

ivankelly · 2018-07-17T11:13:26Z

pulsar-broker/pom.xml

@@ -268,6 +268,12 @@
      <version>${project.version}</version>
      <scope>test</scope>
    </dependency>


We shouldn't need the full s3 dependency now, just whatever AWSCredentials is in.

zhaijack · 2018-07-17T15:01:11Z

retest this please

for C++/Python Tests build error

sijie · 2018-07-17T17:40:59Z

I know it is approved. let's not merge this until I cut 2.1 release.

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](e5b1f7a). Currently it passed the real test in GCS, by test case [ManagedLedgerOffloaderTest#testGcsRealOffload](eda5097#diff-6387a2ab4cf9c9243135b5a34c8522efR584), since lack of GCS mock docker image, will try to use read GCS to do the integration test in later PR. Master Issue: #2067

sijie assigned zhaijack Jul 2, 2018

sijie added the area/tieredstorage label Jul 2, 2018

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

zhaijack force-pushed the jclouds_offloader branch 6 times, most recently from 84e0fbe to 61c1c09 Compare July 4, 2018 07:17

zhaijack force-pushed the jclouds_offloader branch 4 times, most recently from 69edf1d to 808603f Compare July 4, 2018 15:15

zhaijack changed the title ~~WIP-GCS offload support(2): replace s3client api with jclouds related api~~ GCS offload support(2): replace s3client api with jclouds related api Jul 5, 2018

zhaijack force-pushed the jclouds_offloader branch 2 times, most recently from f27b4c2 to 000675e Compare July 5, 2018 14:54

zhaijack changed the title ~~GCS offload support(2): replace s3client api with jclouds related api~~ WIP - GCS offload support(2): replace s3client api with jclouds related api Jul 5, 2018

zhaijack mentioned this pull request Jul 11, 2018

track adobe/s3mock update, since it is not using the latest version of github repo #2133

Closed

zhaijack force-pushed the jclouds_offloader branch from 7fd22ee to 95cff33 Compare July 11, 2018 08:26

zhaijack changed the title ~~WIP - GCS offload support(2): replace s3client api with jclouds related api~~ GCS offload support(2): replace s3client api with jclouds related api Jul 11, 2018

zhaijack mentioned this pull request Jul 13, 2018

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

Merged

ivankelly requested changes Jul 13, 2018

View reviewed changes

zhaijack mentioned this pull request Jul 14, 2018

Offload: BlobStoreTestBase should support to use real AWS if a system property was set #2165

Closed

zhaijack force-pushed the jclouds_offloader branch from e5749a4 to 3e7aad7 Compare July 15, 2018 06:41

ivankelly requested changes Jul 16, 2018

View reviewed changes

jiazhai added 9 commits July 17, 2018 18:47

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

8dacfaa

adobe/s3mock update

c5cfd67

rename S3 before filenames

d844ecc

mv latest docker image of s3mock to apachepulsar

4c1bec4

rename test file name, delete prefix s3

8a07b3a

change log content to avoid print secrets

6468e3e

change following ivan's comments

ff1743f

update License.bin

371be7b

change following ivan's comments, using GetOptions

cf020a7

zhaijack force-pushed the jclouds_offloader branch from 60d6d33 to cf020a7 Compare July 17, 2018 10:51

ivankelly requested changes Jul 17, 2018

View reviewed changes

jiazhai added 2 commits July 17, 2018 19:21

fix rebase error

0f8bc2f

change following ivan's comments

cc73472

ivankelly approved these changes Jul 17, 2018

View reviewed changes

sijie added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Jul 20, 2018

sijie merged commit 3c8d13c into apache:master Jul 20, 2018

GCS offload support(2): replace s3client api with jclouds related api #2065

GCS offload support(2): replace s3client api with jclouds related api #2065

Conversation

zhaijack commented Jul 2, 2018 • edited

sijie commented Jul 2, 2018

zhaijack commented Jul 4, 2018 • edited

zhaijack commented Jul 5, 2018

zhaijack commented Jul 11, 2018 • edited

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

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 15, 2018

Choose a reason for hiding this comment

zhaijack commented Jul 17, 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

zhaijack commented Jul 17, 2018 • edited

sijie commented Jul 17, 2018

GCS offload support(2): replace `s3client` api with `jclouds` related api #2065

GCS offload support(2): replace `s3client` api with `jclouds` related api #2065

zhaijack commented Jul 2, 2018 •

edited

zhaijack commented Jul 4, 2018 •

edited

zhaijack commented Jul 11, 2018 •

edited

zhaijack commented Jul 17, 2018 •

edited