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

Allow users to specify the type of connection to use for s3 transfers. #575

Closed
wants to merge 2 commits into from
Closed

Allow users to specify the type of connection to use for s3 transfers. #575

wants to merge 2 commits into from

Conversation

nasdf
Copy link

@nasdf nasdf commented Nov 13, 2018

This change allows users to specify what type of connection they'd like to use for S3 transfers. For example you may only want to use WiFi to upload larger files.

@minbi
Copy link
Contributor

minbi commented Nov 13, 2018

Hi @nasdf ,

Thanks for the PR we will start reviewing and update the thread afterward.

@minbi minbi added s3 Issues with the AWS Android SDK for Simple Storage Service (S3). pull request labels Nov 13, 2018
@mutablealligator mutablealligator self-assigned this Nov 13, 2018
Copy link
Contributor

@mutablealligator mutablealligator left a comment

Choose a reason for hiding this comment

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

Thank you for submitting this PR. I have left two comments. Once the comments are addressed, I will pull the changes and do some testing.

@danieldekerlegand
Copy link

@kvasukib @cbommas Any updates on this PR? I'm very interested in using this feature in my project. Thanks!

@mutablealligator
Copy link
Contributor

@danieldekerlegand We are still looking into the changes. I will post an update when the PR is merged and released.

@nasdf
Copy link
Author

nasdf commented Dec 10, 2018

Any updates on this?

@frankmuellr
Copy link

Hi @cbommas,

can you please take a look at this PR?

@mutablealligator
Copy link
Contributor

@nasdf I have approved the changes. I will be pulling in the changes and write tests and will be including this in one of the future releases. Thank you!

@mukeshdadarwal
Copy link

@kvasukib Is this feature available now?

@mukeshdadarwal
Copy link

@cbommas Is this feature available now?

@mutablealligator
Copy link
Contributor

@nasdf Sorry to bug you on this. Can you resolve the conflicts and update the PR? I am writing some tests for this change.

@@ -149,11 +149,8 @@ public void onReceive(Context context, Intent intent) {
new Thread(new Runnable() {
@Override
public void run() {
if (networkConnected) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be worked on. The main feature of this class is to check for network connectivity and pause when offline and resume when online.

Copy link
Author

Choose a reason for hiding this comment

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

I moved the network checking logic into the pauseAllForNetwork and checkTransfersOnNetworkReconnect methods so that each transfer can check if its network type is active or not. Does that not work with the updated SDK code?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nasdf Thank you for the information. However my concern is that, when a developer passes the TransferUtilityOptions via TransferUtility object, it is used for the transfers (upload and download). However the NetworkInfoReceiver / TransferService is not aware of that connection type chosen by the developer and we are checking for ANY.

My suggestion would be to find a way to be consistent with the connection type and use that everywhere in the SDK (TransferUtility, Upload/DownloadTask, TransferService and TransferNetworkLossHandler).

I am working on this suggestion and discussing with other folks in the team. I would like to know your approach for this use-case as well.

@mutablealligator
Copy link
Contributor

There are significant changes done in 2.11.0 version of the SDK. The NetworkInfoReceiver from TransferService is decoupled into a standalone component called TransferNetworkLossHandler that can be instantiated without TransferService.

TransferNetworkLossHandler (formerly NetworkInfoReceiver) is a singleton. The connection type is required by TransferNetworkLossHandler to detect the connectivity based on the type. I am looking into the different options on how can the TransferConnectionType be available in TransferNetworkLossHandler.

@bharatUxarmy
Copy link

Hey Guys,

I'm waiting for long enough time to get this feature where we can mark Network type - WiFi or mobile data on uploading. If possible for you guys then please let me know when this feature would be available.

Thanks for your time.

@mutablealligator
Copy link
Contributor

@bharatUxarmy Sorry for the delay. I am working on integrating the changes with the current state of TransferUtility and adding tests. This should be included in the next upcoming release.

@mutablealligator
Copy link
Contributor

Thank you @nasdf for the PR. This change has been released in 2.11.1 version of the SDK. I'm closing the PR as there are significant changes in the codebase which creates conflicts with the PR branch. Your work is highly appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request a new feature s3 Issues with the AWS Android SDK for Simple Storage Service (S3).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants