Google cloud storage support #1340

jkobejs · 2018-11-25T21:07:54Z

Fixes #588

Finished work that @francisdb started on #650.

Implemented missing apis
Added documentation
Improved java/scala dsl's
Added mocked http tests
Wrote tests for client facing apis

I didn't add documentation for all api methods, should add documentation for remaining ones?

lightbend-cla-validator · 2018-11-25T21:07:55Z

Hi @josipgrgurica,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

jkobejs · 2018-11-25T21:26:12Z

I signed the CLA. Sorry, I totally forgot about that.

francisdb · 2018-11-25T21:33:19Z

Thanks for picking this up!

jkobejs · 2018-11-26T07:13:17Z

@francisdb you're welcome. Thank you for doing great work on this connector 👏.

ennru · 2018-12-12T15:43:14Z

Sorry for being slow on reviewing. We focused on Alpakka Kafka for a long while, but are back on this part of Alpakka now.

ennru

Looks great so far I got today. I haven't looked into the internals, yet.

We've recently tried out to use Akka Extensions for connectors that normally have a single incarnation (just as Akka Http contains Http()), that would flip the usage around to do things via methods on a CloudStorage object which implicitly takes the extension. What do you think about such a design? (see #1323 which would change S3 to that style)

...storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/GoogleCloudStorageModel.scala

...a/akka/stream/alpakka/googlecloud/storage/impl/GoogleCloudStorageClientIntegrationSpec.scala

ennru

Looks really great, I've nothing big to object.

google-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/impl/Chunker.scala

google-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/impl/Formats.scala

...e/src/main/scala/akka/stream/alpakka/googlecloud/storage/impl/GoogleCloudStorageClient.scala

ennru · 2018-12-18T13:40:47Z

...rage/src/main/scala/akka/stream/alpakka/googlecloud/storage/javadsl/GoogleCloudStorage.scala

+    }
+}
+
+private[storage] abstract class GoogleCloudStorage {


This cannot be private as it is part of the public API. You should make it a final class by passing in the client and ec.

ennru · 2018-12-18T13:43:07Z

...rage/src/main/scala/akka/stream/alpakka/googlecloud/storage/javadsl/GoogleCloudStorage.scala

+   * Gets information on a bucket
+   *
+   * @param bucketName the name of the bucket to look up
+   * @return a [[CompletionStage]] containing [[Bucket]] if it exists   */


Scaladoc requires the full package path for classes not in the same package (importing doesn't help). But I think its enough to back-tick those as they show clickable in the message signatures.

ennru · 2018-12-18T13:53:36Z

...age/src/main/scala/akka/stream/alpakka/googlecloud/storage/scaladsl/GoogleCloudStorage.scala

+    }
+}
+
+private[storage] trait GoogleCloudStorage {


This can't be private. You should make it a final class by passing in the client.

ennru · 2018-12-18T14:01:55Z

...e-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/StorageSettings.scala

+    new StorageSettings(projectId, clientEmail, privateKey)
+
+  override def toString: String =
+    s"StorageSettings(projectId=$projectId, clientEmail=$clientEmail, privateKey=$privateKey"


Don't print out the key by mistake...

Suggested change

s"StorageSettings(projectId=$projectId, clientEmail=$clientEmail, privateKey=$privateKey"

s"StorageSettings(projectId=$projectId, clientEmail=$clientEmail, privateKey=***)"

ennru · 2018-12-18T14:07:55Z

google-cloud-storage/src/test/scala/docs/scaladsl/ExampleUsage.scala

+  val chunkSize = 5 * 1024 * 1024
+
+  val uploadSink: Sink[ByteString, Future[StorageObject]] =
+    storage.resumableUpload(bucketName, objectName, ContentTypes.`text/plain(UTF-8)`, chunkSize)


In a perfect world, we'd pull these from sources that are run within the tests.

josefelixh · 2019-01-28T22:20:26Z

Is there any plans to continue working on this PR?

jkobejs · 2019-01-29T15:19:54Z

Is there any plans to continue working on this PR?

Yes there are, I plan to continue by the end of this week. 😄

ennru · 2019-05-22T05:51:25Z

Hi @josipgrgurica,

Do you still have plans to continue this? Or would you like anybody else to carry on?

jkobejs · 2019-05-28T14:56:11Z

Hi @josipgrgurica,

Do you still have plans to continue this? Or would you like anybody else to carry on?

Hi, I'm sorry about not pushing anything, I got stuck in my daily job 😞
I plan continue developing this feature 😄

I pushed code with changes for most comments.

jkobejs · 2019-05-28T15:01:16Z

Looks great so far I got today. I haven't looked into the internals, yet.

We've recently tried out to use Akka Extensions for connectors that normally have a single incarnation (just as Akka Http contains Http()), that would flip the usage around to do things via methods on a CloudStorage object which implicitly takes the extension. What do you think about such a design? (see #1323 which would change S3 to that style)

That makes sense to me, is this PR that I should use for reference #1395 ?

jkobejs · 2019-05-28T15:14:31Z

...loud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/impl/GoogleSession.scala

+  private def expiresSoon(g: AccessTokenExpiry): Boolean =
+    g.expiresAt < (tokenApi.now + 60)
+
+  def getToken()(implicit materializer: Materializer): Future[String] = {


I'm using this lib in production for few months and I noticed that this call can fail, it is transient issue. Does it make sense to retry it?

Possibly, but it is important to back off appropriately.
I'm not aware that someone had that issue with the other Google Cloud connectors, though.

You are right, we should be careful with back off.
What do you suggest for defaults?

jkobejs · 2019-06-06T07:17:19Z

@ennru I refactored code to use Akka Extensions, I took current S3 api design as a guide.

ennru

Had another look, still impressed. Great that you added the extension.

google-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/impl/Formats.scala

ennru · 2019-06-13T17:22:37Z

...loud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/impl/GoogleSession.scala

+  private def expiresSoon(g: AccessTokenExpiry): Boolean =
+    g.expiresAt < (tokenApi.now + 60)
+
+  def getToken()(implicit materializer: Materializer): Future[String] = {


Possibly, but it is important to back off appropriately.
I'm not aware that someone had that issue with the other Google Cloud connectors, though.

...ud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/impl/GCStorageStream.scala

ennru · 2019-06-13T17:51:29Z

...ud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/impl/GCStorageStream.scala

+                             contentType: ContentType,
+                             chunkSize: Int): Flow[ByteString, (HttpRequest, (MultiPartUpload, Int)), NotUsed] = {
+
+    assert(


This assertion could go all the way up to resumableUpload AFAICS.

ennru · 2019-06-13T18:01:22Z

...cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/GCStorageSettings.scala

+    val projectId = c.getString("project-id")
+    val clientEmail = c.getString("client-email")
+    val privateKey = c.getString("private-key")
+    val baseUrl = if (c.hasPath("base-url")) c.getString("base-url") else defaultBaseUrl


The idea of using config is to have all defaults in reference.conf so that they are always set.

ennru · 2019-06-13T18:04:20Z

The build fails on MiMa, you need to add .disablePlugins(MimaPlugin) to the project in build.sbt for now.

lightbend-cla-validator · 2019-06-17T18:43:00Z

Hi @jkobejs,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

2m

Looking really good. Just some nitpick comments.

2m · 2019-06-19T13:11:42Z

...cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/javadsl/GCStorage.scala

+    GCStorageStream.listBucket(bucket, Option(prefix)).asJava
+
+  /**
+   * Downloads object from bucket. Returns an empty Source if the object was not found.


This is contradictory with the comment in the "@return" section.

2m · 2019-06-19T13:17:42Z

google-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/GCStorageExt.scala

+import akka.actor.{ActorSystem, ExtendedActorSystem, Extension, ExtensionId, ExtensionIdProvider}
+
+/**
+ * Manages one [[S3Settings]] per `ActorSystem`.


Suggested change

* Manages one [[S3Settings]] per `ActorSystem`.

* Manages one [[GCStorageSettings]] per `ActorSystem`.

2m · 2019-06-19T13:22:03Z

google-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/BucketInfo.scala

@@ -0,0 +1,10 @@
+/*


This file should be moved to impl package, as it is only accessed from there.

2m · 2019-06-19T13:22:13Z

...-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/BucketListResult.scala

@@ -0,0 +1,21 @@
+/*


This file should be moved to impl package, as it is only accessed from there.

2m · 2019-06-19T13:23:37Z

google-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/FailedUpload.scala

+
+import scala.collection.immutable.Seq
+
+final case class FailedUpload(reasons: Seq[Throwable]) extends Exception(reasons.map(_.getMessage).mkString(", "))


This should be converted to non-case class and introduced a Java API getter for reasons (as other model classes have).

2m · 2019-06-19T13:24:47Z

...e-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/MultiPartUpload.scala

@@ -0,0 +1,10 @@
+/*


This file should be moved to impl package, as it is only accessed from there.

2m · 2019-06-19T13:25:49Z

...loud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/UploadPartResponse.scala

@@ -0,0 +1,22 @@
+/*


This file should be moved to impl package, as it is only accessed from there.

2m · 2019-06-19T13:26:18Z

...e-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/RewriteResponse.scala

@@ -0,0 +1,17 @@
+/*


This file should be moved to impl package, as it is only accessed from there.

2m · 2019-06-19T13:28:35Z

google-cloud-storage/src/main/resources/reference.conf

@@ -0,0 +1,6 @@
+alpakka.googlecloud.storage {


Other Google Cloud connector (Pub/Sub) has used alpakka.google.cloud.pubsub namespace. So this one should follow suit.

2m · 2019-06-20T11:53:24Z

project/Dependencies.scala

@@ -22,6 +22,8 @@ object Dependencies {
  val CouchbaseVersion = "2.7.2"
  val CouchbaseVersionForDocs = "2.7"

+  val JwtCoreVersion = "2.1.0"


The 2.13 build failed because we upgraded from Scala 2.13.0-M5 to 2.13.0 in master and jwt-core 2.1.0 is not available for Scala 2.13.0. It is a bit tricky to upgrade jwt-core for libraries that are using it already (#1762 and #1763), but since this is a new connector, this can be upgraded to 3.0.1 safely.

I updated JwtCoreVersion to 3.0.1 together with failing code.

I noticed these warnings when compiling against Scala 2.13.0

[warn] /Users/jgrgurica/Development/FOOS/alpakka/google-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/FailedUpload.scala:15:47: object JavaConverters in package collection is deprecated (since 2.13.0): Use `scala.jdk.CollectionConverters` instead [warn] def getReasons: java.util.List[Throwable] = reasons.asJava [warn] ^ [warn] /Users/jgrgurica/Development/FOOS/alpakka/google-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/FailedUpload.scala:23:65: object JavaConverters in package collection is deprecated (since 2.13.0): Use `scala.jdk.CollectionConverters` instead [warn] def create(reasons: java.util.List[Throwable]) = FailedUpload(reasons.asScala.toList) [warn] ^ [warn] /Users/jgrgurica/Development/FOOS/alpakka/google-cloud-storage/src/main/scala/akka/stream/alpakka/googlecloud/storage/impl/GCStorageStream.scala:598:27: value Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream [warn] .mapConcat(r => Stream.continually(r)) [warn] ^ [warn] three warnings found

Can you advise me how to deal with these warnings since scala.jdk.CollenctionConverters and scala.collection.immutable.LazyList doesn't exist in previous versions of Scala?

We will have to deal with warnings for now. It will be possible to get rid of those when a new scala-collection-compat release is out with scala/scala-collection-compat#217

2m

This one looks good to be merged in. I'll squash the commits to one, that contains all @francisdb work, and to another one with all @jkobejs work. And after last Travis validation run wel'll merge it.

Implement missing apis Add documentation Improve java/scala dsl's Add mocked http tests Write tests for client facing apis Add stroage test command into travis script Refactor api to static methods remove VS settings, format dependencies, add project info add reference conf, log status codes, disable mima Bump jwt-core to 3.0.1

2m · 2019-06-25T05:37:27Z

Failure was #1417

2m · 2019-06-25T07:30:32Z

Great to have this in. Thanks a lot @francisdb and @jkobejs!

francisdb · 2019-06-25T08:33:40Z

Great to see @jkobejs took the time to complete this, thanks!

ennru · 2019-06-25T08:47:44Z

I couldn't agree more, this is what open source is about. Work together.

jkobejs · 2019-06-26T06:26:02Z

@francisdb thanks for opportunity to finish your work, it has been a pleasure working on this!

ennru added the p:new label Nov 26, 2018

ennru reviewed Dec 12, 2018

View reviewed changes

ennru reviewed Dec 18, 2018

View reviewed changes

jkobejs commented May 28, 2019

View reviewed changes

ennru reviewed Jun 13, 2019

View reviewed changes

2m reviewed Jun 19, 2019

View reviewed changes

probot-autolabeler bot added the documentation label Jun 20, 2019

2m reviewed Jun 20, 2019

View reviewed changes

2m approved these changes Jun 21, 2019

View reviewed changes

francisdb and others added 3 commits June 21, 2019 15:44

initial commit of googlecloudstorage

246cabe

Fixups after rebase

eeac4a4

2m force-pushed the googlestorage branch from 066d265 to eeac4a4 Compare June 21, 2019 13:06

ennru added the p:google-cloud-storage label Jun 24, 2019

2m merged commit be3b89a into akka:master Jun 25, 2019

2m added this to the 1.0.3 milestone Jun 25, 2019

francisdb mentioned this pull request Jul 5, 2019

WIP - Adds Google cloud storage support #650

Closed

	s"StorageSettings(projectId=$projectId, clientEmail=$clientEmail, privateKey=$privateKey"
	s"StorageSettings(projectId=$projectId, clientEmail=$clientEmail, privateKey=***)"

	* Manages one [[S3Settings]] per `ActorSystem`.
	* Manages one [[GCStorageSettings]] per `ActorSystem`.


		import scala.collection.immutable.Seq

		final case class FailedUpload(reasons: Seq[Throwable]) extends Exception(reasons.map(_.getMessage).mkString(", "))

Google cloud storage support #1340

Google cloud storage support #1340

Conversation

jkobejs commented Nov 25, 2018

lightbend-cla-validator commented Nov 25, 2018

jkobejs commented Nov 25, 2018

francisdb commented Nov 25, 2018

jkobejs commented Nov 26, 2018

ennru commented Dec 12, 2018

ennru left a comment

Choose a reason for hiding this comment

ennru left a 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

josefelixh commented Jan 28, 2019

jkobejs commented Jan 29, 2019

ennru commented May 22, 2019

jkobejs commented May 28, 2019

jkobejs commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkobejs commented Jun 6, 2019 • edited Loading

ennru left a 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

ennru commented Jun 13, 2019

lightbend-cla-validator commented Jun 17, 2019

2m left a 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

2m left a comment

Choose a reason for hiding this comment

2m commented Jun 25, 2019

2m commented Jun 25, 2019

francisdb commented Jun 25, 2019

ennru commented Jun 25, 2019

jkobejs commented Jun 26, 2019

jkobejs commented Jun 6, 2019 •

edited

Loading