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

UploaderService is now a thing. #180

Merged
merged 8 commits into from Nov 30, 2014

Conversation

Projects
None yet
3 participants
@linster
Copy link
Contributor

linster commented Nov 30, 2014

Created a Service. Refactored the completeQueuedTasks() to use polymorphism. Switching on a type was evil, so I created a retry() method in each task. The tasks that need to be retried implement the abstract method.

Little more refactoring in each task so that instead of calling EventBus directly, they call a method in AbstractDataManagerTask to tryPushLater(). This has the effect of calling DataManager.startUploaderService().

Please, don't start UploaderService yourself. I have no idea what happens if two of them are running at once. Use DataManager.startUploaderService() if you really have to (but you shouldn't).

Also, did a bunch of magic with concurrent collections so that it should work. Used a BlockingQueue instead of an ArrayList in the EventBus. Synchronized keywords abound. Also, some neat queue stuff in UploaderService so we don't have an infinite loop trying to upload stuff while offline.

Loads and loads of comments everywhere explaining magic things I think were a good idea at the time :P.

@stephenjust should look at how I check network connectivity in remoteDataStore using the hasAccess() method. It may make sense to replace what I put in there with a dummy call to ElasticSearch and see if it works.

linster added some commits Nov 29, 2014

Removed CompleteQueuedEvents() from DataManager
Started working on UploaderService. Seems like everytime Android tries
to help you (like with IntentService), it's severely limited and doesn't
do what you want.
Commit before changing interface.
AbstractEvent will gain a Retry() method. This way, the QueueClearer can
use polymorphism instead of a horrible switch-case.
Added a retry() method to Tasks
No more horrible switches on types. Polymorphism ftw.

If you are implementing a new task in the system, don't worry about what
DataManager dm is for the retry() method. UploaderService supplies it
when needed.
Changed the eventbus to use a BlockingQueue.
BlockingQueue has some nice thread-safe features.  Read the huge block
comment on what I did inside completeQueuedEvents() in the QueueCleaner.
There's some very fancy footwork to prevent infinite looping while
offline.
More updates.
RemoteDataStore.hasAccess() now does stuff and is required.
@stephenjust, take a look in UploaderService.java to see how its needed.
I'm not sure trying something network-related, then catching an
IOException within the logic I have is better than just having
.hasAccess() poll the system.

There's a little bit of sleeping evilness on how the service runs. If
offline, it sleeps for a while, then tries again. Not ideal, but we're
running out of days to do StopServiceOnResult(int), then restart the
service later, and do fancy stuff.
@stephenjust

This comment has been minimized.

Copy link

stephenjust commented on 591df03 Nov 30, 2014

I don't necessarily agree that the network connectivity check belongs in RemoteDataStore as it doesn't actually tell you if you can access the datastore or not. The server could be down, you could have a network connection, but no access to the internet, etc. This is why I never used hasAccess (and I think I removed it at least once).

As you have also identified, polling is probably not ideal. Take a look at this Stack Overflow question: http://stackoverflow.com/questions/1783117/network-listener-android - this means that every time the network state changes, you can actually have the system notify your app rather than you asking the system.

linster added some commits Nov 30, 2014

Moved startUploaderService() back to DataManager
The method will live here.

The tasks are about to be refactored. AbstractDataManagerTask will gain
a method that does the equivalent of all the EventBus....addEvents()
now, but will also run DataManager.startUploaderService().  I didn't
want to pollute EventBus with holding an application context and
overload it into starting services.
Little bit of shotgun surgery
The individual tasks no longer talk to the eventBus, but use a method in
AbstractDataManagerTask to do the same thing. This new method,
tryPushLater(), also starts the uploaderService.

@linster linster referenced this pull request Nov 30, 2014

Closed

Uploader Service #173

// TODO Auto-generated method stub
return false;
//Needed in UploaderService.
//Used code from here: http://www.vogella.com/tutorials/AndroidNetworking/article.html#networkstate

This comment has been minimized.

@stephenjust

stephenjust Nov 30, 2014

Member

http://www.vogella.com/legal.html - If you end up using this, you have to make note in the readme or credits file or whatever it is EPL requires.

Fixed hasAccess() thing
UploaderService just sleeps.
@stephenjust

This comment has been minimized.

Copy link
Member

stephenjust commented Nov 30, 2014

Good enough for now. I wonder how much this one will break 👍

@linster

This comment has been minimized.

Copy link
Contributor Author

linster commented Nov 30, 2014

If it doesn't actually have to be thread-safe (since AsyncTasks run on an Executor) and since BlockingQueue is designed for producer-consumer problems, it should be okay. Hopefully being overzealous with synchronized didn't create deadlocks :P

@RyanKusters

This comment has been minimized.

Copy link
Contributor

RyanKusters commented Nov 30, 2014

I don't see any problems. Only Human.

RyanKusters added a commit that referenced this pull request Nov 30, 2014

Merge pull request #180 from linster/master
UploaderService is now a thing.

@RyanKusters RyanKusters merged commit e702c49 into CMPUT301F14T14:master Nov 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment