search

mikehardy / google-analytics-java

Open Source license compatible Java API for Google Analytics
9 stars 3 forks source link

Make HttpClient async #24

Closed jtjeferreira closed 5 years ago

jtjeferreira commented 5 years ago

fixes #23

mikehardy commented 5 years ago

Aw man, this is tough for me. First, thanks for contributing at all, seriously. And technically, this seems like clearly the right way to go - no argument.

But my primary use case as a consumer of this library is an Android app that needs to support APIs down to 15, and Android only gets the CompletableFuture implementation in API24. It's going to be a long time before that library consumer can adopt a CompletableFuture-based implementation - long enough that I don't want to have for instance a 1.x here that isn't async and a 2.x that is or anything.

I'd like to make this work if possible though. Do you have any ideas that could accommodate this CompletableFuture async implementation as an option such that it could be adopted by those with modern API support but wasn't mandatory for those still using prehistoric JDKs? Perhaps a parallel set of APIs (even though our namespace is a bit polluted by use of the "Async" word already - I'd be okay with changing or adding names in the API and doing a major version)

jtjeferreira commented 5 years ago

Hi, I am no Android developer but maybe this can help https://retrostreams.github.io/android-retrofuture/?

mikehardy commented 5 years ago

Hmm - let me check into that - those look like they'd fit perfectly.

The last time I looked at one of these Java feature backport projects the project itself looked fine but the toolchain integration here seemed difficult, and it was far easier to just remove the API use here since functionality was equivalent.

Not so in this case, so I'm willing to use one of these libraries but I have not successfully integrated one yet and I'm a bit busy right now.

Are you okay with waiting for a while until I get a chance to come back here and wrestle with the build system to see if I can get it to work?

If you're building something that strictly requires this functionality you can assume that at minimum I'm willing to accept the functionality in the form of separate APIs, so hopefully that's somewhat useful for you

Cheers

jtjeferreira commented 5 years ago

Would be great if you could help with build part, but yeah i don't have any rush to change what I currently have...

mikehardy commented 5 years ago

Progress update - had a moment today and I tried the naive method of integrating this into my android project - simply adding the retrofuture dependency there, and it didn't work, so I think this will have to be a compile-time step here during library generation in order for it to work. Should still be a possibility, just need to integrate it with maven during build

mikehardy commented 5 years ago

I see what's needed now. In order for this to work on Android the PR itself needs to be altered so that it uses the back-ported interfaces (typically that means importing 'java8.util.concurrent.CompletableFuture' or similar) from these packages:

https://github.com/stefan-zobel/streamsupport/blob/master/src/cfuture/java/java8/util/concurrent/CompletableFuture.java

After adding this to pom.xml:

        <dependency>
            <groupId>net.sourceforge.streamsupport</groupId>
            <artifactId>streamsupport-cfuture</artifactId>
            <version>1.7.0</version>
        </dependency>

That way all the code required for it to work on Android (which is missing this part of Java8) actually comes from the backport library implementation.

If this PR is re-worked to use the backport library implementation I'll retest it against android, and if it works (it should?) I'll merge it in

mikehardy commented 5 years ago

Also, please git fetch the lastest master and rebase against it - I forgot to git add / git commit one test (for the sampling strategy) when I did the last release, and of course when I pulled your branch to work on it, that test was failing :-). My error, but it'll be easier if you've got latest master. Thanks!

jtjeferreira commented 5 years ago

Hi. I will try to have a look into this in the next few days.

jtjeferreira commented 5 years ago

So I did what you asked:

This should make the library usable from old android versions.

However, this is adding extra dependencies for users of recent android versions and users of java8, so this was not feeling right to me so I started investigating how other libraries are solving this. I found several alternatives:

I think the "Separate distribution" alternative is what you wanted to avoid in the beginning, so WDYT about proguard?

mikehardy commented 5 years ago

Thank you the continued effort, and yes I have to admit the whole thing "feels" wrong. I appreciate your suffering through it with me.

If I weren't stuck with Android dependencies I'd move immediately, java 7 is kind of ridiculous as a limit. Unfortunately, the "recent" Android versions that support it are less than half my userbase of a million users, mostly (from what I can tell by my stats) either medical students studying to be doctors or Portuguese citizens learning English. It's a flashcard app (AnkiDroid) and it's open source and I can't bring myself to shut half of those folks out so I'm struggling for something that works, even if it is a little ugly.

Unfortunately with regard to ProGuard, it's phasing out of the Android toolchain to be replaced by "D8" in the Android Studio release that just went to RC1 status and just in general I don't want to rely on the Android Studio toolchain for things Google isn't personally invested in.

So maybe it could be a branch as you mention. The changes aren't huge between the two styles I don't think? We could do it the clean way on master and I could keep an "AndroidPre24" fork/distribution next to it? Easy enough for me I think.

So I'd merge this, test it quickly, then we could convert to the clean / Java8+ way on master and be done?

jtjeferreira commented 5 years ago

If I weren't stuck with Android dependencies I'd move immediately, java 7 is kind of ridiculous as a limit. Unfortunately, the "recent" Android versions that support it are less than half my userbase of a million users

I completely understand the reason for not dropping the old versions support

So I'd merge this, test it quickly, then we could convert to the clean / Java8+ way on master and be done?

If this solution works for you. It also works for me

The changes aren't huge between the two styles

Yes, it is just a matter of using java8.util.concurrent instead of java.util.concurrent

mikehardy commented 5 years ago

Thanks again for your understanding and your patience. I just built this and tested it and despite knowing exactly how they are doing it, I'm still sort of surprised this black magic works just fine. All the way down to API18 in my testing so far, where API18 broke before. So I'll finish my testing but I'm sure this will merge in a little bit. Then I'll set up the Java7 branch and document things etc then make master "clean".

If I have trouble with that for some reason I'll tag you and ask for help, but otherwise this looks ready to go and I really appreciate the contribution - thanks!

mikehardy commented 5 years ago

All done and available as version 2.0.6 on maven-central

https://github.com/mikehardy/google-analytics-java/releases