Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP - Adds Google cloud storage support #650

Closed
wants to merge 9 commits into from
Closed

WIP - Adds Google cloud storage support #650

wants to merge 9 commits into from

Conversation

francisdb
Copy link
Contributor

@francisdb francisdb commented Dec 14, 2017

Superseded by #1340

fixes #588

This is an initial import of the code we currently have in (pre)production at https://www.waylay.io . Kudos to @brunoballekens

Some questions toward maintainers:

  • Can we keep using play-json?
  • Can we keep using specs2?
  • Is it a good idea to make a difference between Source.empty (404) and Source.single(ByteString.empty) (empty file)? Other solution would be returning Future[Option[Source[ByteString], Metadata]]] which makes things rather ugly
  • Do you have a guide on best practices for coming up with a java api?
  • Please review TODO's to see if anything is missing

@ennru ennru added the p:new label Dec 15, 2017
@francisdb
Copy link
Contributor Author

@ennru mind answering the questions I put in the comment?

@ennru
Copy link
Member

ennru commented Dec 19, 2017

Great that you'd like to donate this code to Alpakka.

  • Akka Http's spary-json would be a more "natural" choice. Does it provide all you need?
  • Scalatest is preferred as we've no other tests in specs2. Is is too much work to migrate?
  • I'd say the distinction between not found and empty file is important.
  • We don't really have a guide how to offer a Java API. We strive for feature equality with Java-esque interface (Optional, java.util.List).

I'll need to look at the code to give more guidance...

@francisdb
Copy link
Contributor Author

I'll switch to scalatest and spray-json, should not be that much work

@johanandren
Copy link
Member

Would be good if you could also read through the brand new contributor advice https://github.com/akka/alpakka/blob/master/contributor-advice.md and adjust public/internal APIs etc as needed, thanks!

@francisdb
Copy link
Contributor Author

@ennru @johanandren I would like to share the Session class from the googlecloud-pub-sub project. Any recommendations there?

see also GoogleSession in #764

@ennru
Copy link
Member

ennru commented Feb 12, 2018

Yes, sharing sounds good to me. How much code could be shared? Is it "just" the session class?

We'd either need to add a google-cloud-base module or let one of the connectors be the "main" module and let the others depend on it.

@tg44
Copy link
Contributor

tg44 commented Feb 13, 2018

Yapp sharing could be good. Centralize the deps, create a "common" way to do things. But on the other hand we are speaking about ~90 line of code. So I still not convinced myself.
The only thing we shouldn't do; is using the official google sdk-s as deps. I walked through some of their code and trust me, you really don't want to depend on that :)

So I think for the start we could implement the same session and token handling for the 3 modules, and we should generalize/centralize this later. (Mostly because if we do a refactor in one PR we need to do that on the other related PRs too. If we have a more "stable/freezed" codebase we could refactor more easily.) Maybe start a new issue with this "google shared code" problem, and we will not forget it.

@francisdb
Copy link
Contributor Author

@tg44 makes sense to wait until everything has been merged

@@ -16,11 +16,11 @@ class SessionSpec extends WordSpec {
"report the path of the missing service account file" in {
implicit val as: ActorSystem = null
implicit val mat: Materializer = null
val session = new Session(GoogleAuthConfiguration("missingpath"), Seq.empty)
val session = new Session(GoogleAuthConfiguration("/missing/path/test.json"), Seq.empty)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this break the build on non unixy machines. Use TempDirectory.location() and then combine with a more specific file name that will not exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to Paths.get(System.getProperty("java.io.tmpdir")).resolve("idonotexist.json") as TempDirectory is not accessible

@francisdb
Copy link
Contributor Author

I don't have the time to work on this any more, might in the end be a better idea to create a wrapper for the official google client that now supports async callbacks?

@francisdb francisdb closed this Jul 5, 2018
@francisdb
Copy link
Contributor Author

on the other hand we have been running this code in production for a while now and it does its job

@ennru
Copy link
Member

ennru commented Jul 6, 2018

Sorry, to see you leaving this behind. Welcome back another day.

@l15k4
Copy link

l15k4 commented Sep 21, 2018

We are using it too in production

@jkobejs
Copy link
Contributor

jkobejs commented Oct 19, 2018

@ennru what needs to be implemented to have this PR ready for merge?
Is there anything else besides:

  • Documentation
  • Create proper java/scala dsl's
  • Add mocked http tests
  • Review configuration compared to other google module
  • Switch to Host-Level Client-Side API instead of Request-Level? (Maybe)

francisdb mentioned that it might be better idea to implement this in terms of new google client that supports async callbacks, it that better approach?

@francisdb can you point me to the google client that supports async callbacks, I haven't had luck to find documentation about it yet.

@tg44
Copy link
Contributor

tg44 commented Oct 21, 2018

I have no idea which one is the "new google client", but last time when I checked out the official java sdk from google, it was really ugly. Personally I would not use that lib in any production system. (It makes a lot of unneeded requests, no reasonable caching, the codebase is hardly readable and not segmented, and if you check the dependencies its really big.) So we coded all the required functionality from the REST documentation after we tried to use it and failed bcs of the performance and memory issues.

@francisdb
Copy link
Contributor Author

I guess I mixed op Amazon and Google. There is a new async aws client. So forget about that comment.

@ennru
Copy link
Member

ennru commented Oct 31, 2018

@josipgrgurica Would be great if you have the time to get this rolling again. We'd like it to follow the structure as described in the contributor advice.

@jkobejs
Copy link
Contributor

jkobejs commented Oct 31, 2018

@ennru I have time to continue on this, I'll go through this PR and contributor advice to see what is missing and what needs to be changed. Should I fork francisdb:googlestorage branch and work on it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Google cloud storage client
6 participants