Network Reachability

[smurthas]

This checks for an available network connection before attempting an upload. It does not do any sort of quality or speed check.

The unit test only exercises the the logic of calling the isNetworkConnected method (I hand verified in an Android app and a small Java program as well).

Some things to consider:

• Does the strategy of holding on to the Builder object in the client and using the abstract isNetworkConnected method to deal with the differences between the two platforms make sense and fit well with the other "platform differences" patterns in the codebase?
• The Java SDK "defaults" to not checking the network status since this is much less of a concern on the server-side. For completeness and consistency, I included a simple (but not very efficient) loop that will only run if a specific network interface name is set via the JavaKeenClientBuilder.setNetworkInterfaceName method. Does it even make sense to include this functionality? The only scenario I can envision is a client-side, old-school Java application, but do those still exist? Even if they do, I'm not really certain this is the right pattern (the client app is unlikely to know which interface to check for -- maybe better to just have a boolean flag and skip the loopback interface since that is always "connected"?).
• The loop in the JavaKeenClientBuilder.isNetworkConnected method might be inefficient. If we decide to keep that functionality, we should consider caching a pointer to the interface.
• The loop in the JavaKeenClientBuilder.isNetworkConnected method fails silently. Should this check for debug mode == false and throw the error? (I didn't really look into this as I think we might just ditch that section of code.)
• I could honestly use a sanity check on my private/protected/package/public usage as my head has been buried in JS for too long!

[joshed-io]

Private/public scopes look good to me.

Those are some important considerations. I can see how checking network could be unpredictable on certain platforms. That said, this is a marked improvement over what we do now, and the only way we'll learn about platform difference is to observe it in the wild.

:+1:

[Geeber]

Hey Simon, just wanted to let you know that I've started looking at this but it's going to take me a bit to digest. I'll try to get you comments by EOD Monday at the latest, but hopefully sooner.

[smurthas]

Sounds good, thanks for the status update.

[Geeber]

OK, so I think this is looking pretty good. The main issue we need to address is the persistence of the Builder object. What if instead we added another interface (e.g. NetworkManager), in the same way that we use HttpHandler, KeenJsonHandler, etc.? Then the builder could hand off the interface instance to the client and be safely GC'd.

If we introduce a NetworkManager interface (or something similar), we should think ahead a bit to what we'd need to use it for network rate throttling. If we renamed isNetworkConnected to something like isNetworkUsable or shouldRequestBeSent (or something), then that could handle both throttling and connectivity, but we'd also need to have another method for reporting usage. It gets a little complicated; on second though maybe we should hold off and plan on adding that in 3.0. Still good to start noodling on a little.

As for the Java client, I think you're right that there isn't much need for network checks there. The implementation is a little messy and the value is marginal at best, so I'm inclined to simplify our lives and just cut it. If we provide default behavior assuming the network is always on, plus an interface which somebody can override if they really want to, then I think we're good. Should a vocal feature request ever come in, we at least have a good starting point in the GitHub history ;)

Also, we will want to try and make sure that we do this in a backwards-compatible way if possible. Bumping the version to 3.0 isn't out of the question but if we bump major versions too often it will become an annoyance for our users.

Anyway, let me know what you think about the interface idea. I'm happy to discuss in more detail or brainstorm other options if you'd like.

[smurthas]

I was initially trying to avoid adding another interface, but I think you are right, that is a cleaner way to handle this. Since it will be internal, I'd opt to have it be simple to start and just have the existing isNetworkConnected function. As we understand the implementation details of the other checks (bandwidth et al), we can decide whether those should be wrapped up in a single function or wether they should be broken out and called separately from KeenClient.sendQueuedEvents.

Does that sound good?

[Geeber]

Yeah, I think it's fine to keep it simple for now. The only reason I was trying to think ahead a little is that once we introduce the interface, then adding methods to it becomes backwards-incompatible and would require a version bump. But I think all of the throttling stuff is far enough out that it makes sense to target it for a 3.0 release anyway. (As an aside, it might be worthwhile at some point to work on a feature/release road map. Let's worry about that after some of this low-hanging fruit is taken care of though.)

[smurthas]

Ok, I've made improvements based on all of your feedback @Geeber. Take another look when you get a chance and let me know if my updates make sense or if there is anything else that catches your eye.

[Geeber]

This all looks good, just a few minor cosmetic things. Let me know when it's ready. We may want to make a 2.1 branch to merge into though, if we want to roll this in with the max attempts work. We can figure that out later this week.

[smurthas]

k, should be all set now.

[smurthas]

@Geeber Can we merge this so I can rebase #28 and we can start to review that?

[Geeber]

Yep I'm gonna create a 2.1 branch and merge this into that. I'll also try and do at least a first pass review of #28. Should get to that this morning.

[Geeber]

I created the branch 2.1.0, but apparently there's no way to modify the target of an existing PR (see https://github.com/isaacs/github/issues/18). Can you go ahead and merge into the branch locally and push? Or close this PR and create a new one targeting that branch, whichever is easier.

[Geeber]

Also I just added you as a collaborator (I think?) so you should be able to do some of this yourself now. It's clear that I don't have the bandwidth to keep up with you and I don't want you to be blocked on me. Let me know if the permissions still don't work for some reason :)

[smurthas]

This is merged into 2.1.0. https://github.com/smurthas/KeenClient-Java/compare/keenlabs:2.1.0...reachability