-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
5128: Fix stuck uploads #5257
5128: Fix stuck uploads #5257
Changes from 19 commits
4aa4ea0
96058ff
870f286
1ebb117
2dd2be1
6b9e5ab
2ec4667
f1b2e03
0a64989
c785d6f
e1855b9
6d013e6
4cafd2a
04b7255
533e976
4fbe261
c35d936
d1bfb48
3ff04d3
ab2f114
faf0cde
f441eaa
5ec4d56
034d8d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,7 @@ public class ContributionsFragment | |
private static final String CONTRIBUTION_LIST_FRAGMENT_TAG = "ContributionListFragmentTag"; | ||
private MediaDetailPagerFragment mediaDetailPagerFragment; | ||
static final String MEDIA_DETAIL_PAGER_FRAGMENT_TAG = "MediaDetailFragmentTag"; | ||
private static final int MAX_RETRIES = 10; | ||
nicolas-raoul marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
@BindView(R.id.card_view_nearby) public NearbyNotificationCardView nearbyNotificationCardView; | ||
@BindView(R.id.campaigns_view) CampaignView campaignView; | ||
|
@@ -593,6 +594,15 @@ public void notifyDataSetChanged() { | |
} | ||
} | ||
|
||
/** | ||
* Restarts the upload process for a contribution | ||
* @param contribution | ||
*/ | ||
public void restartUpload(Contribution contribution) { | ||
contribution.setState(Contribution.STATE_QUEUED); | ||
contributionsPresenter.saveContribution(contribution); | ||
Timber.d("Restarting for %s", contribution.toString()); | ||
} | ||
/** | ||
* Retry upload when it is failed | ||
* | ||
|
@@ -601,10 +611,22 @@ public void notifyDataSetChanged() { | |
@Override | ||
public void retryUpload(Contribution contribution) { | ||
if (NetworkUtils.isInternetConnectionEstablished(getContext())) { | ||
if (contribution.getState() == STATE_FAILED || contribution.getState() == STATE_PAUSED || contribution.getState()==Contribution.STATE_QUEUED_LIMITED_CONNECTION_MODE) { | ||
contribution.setState(Contribution.STATE_QUEUED); | ||
contributionsPresenter.saveContribution(contribution); | ||
Timber.d("Restarting for %s", contribution.toString()); | ||
if (contribution.getState() == STATE_PAUSED || contribution.getState()==Contribution.STATE_QUEUED_LIMITED_CONNECTION_MODE) { | ||
restartUpload(contribution); | ||
} else if (contribution.getState() == STATE_FAILED) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to test the change in my device. Blindly retrying all failed uploads does seem to be affecting the UX badly. The app seems to just retrying genuinely failed uploads again-and-again each time I open the app. Is there really no other attribute that we have that could help with identifying if an upload failed for a genuine reason? (I'm presuming its because of beta cluster not supporting stashed uploads). Also, I remember cancelling all uploads manually one time. The cancel didn't seem to have worked. Has anyone else observed this too? |
||
int retries = contribution.getRetries(); | ||
/* Limit the number of retries for a failed upload | ||
to handle cases like invalid filename as such uploads | ||
will never be successful */ | ||
if(retries < MAX_RETRIES) { | ||
contribution.setRetries(retries + 1); | ||
Timber.d("Retried %d times", retries + 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can it be changed to "Retried upload %d times" so it's clear what we're trying to retry? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed the debug log line could say that, and also say what file, because often several files are being retried at the same time. |
||
restartUpload(contribution); | ||
} else { | ||
// TODO: Show the exact reason for failure | ||
Toast.makeText(getContext(), | ||
R.string.retry_limit_reached, Toast.LENGTH_SHORT).show(); | ||
} | ||
} else { | ||
Timber.d("Skipping re-upload for non-failed %s", contribution.toString()); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,20 @@ | ||
package fr.free.nrw.commons.upload; | ||
|
||
import static fr.free.nrw.commons.contributions.ContributionController.ACTION_INTERNAL_UPLOADS; | ||
import static fr.free.nrw.commons.upload.UploadPresenter.COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES; | ||
import static fr.free.nrw.commons.wikidata.WikidataConstants.PLACE_OBJECT; | ||
|
||
import android.Manifest; | ||
import android.annotation.SuppressLint; | ||
import android.app.ProgressDialog; | ||
import android.content.Intent; | ||
import android.os.Bundle; | ||
import android.provider.Settings; | ||
import android.util.DisplayMetrics; | ||
import android.view.View; | ||
import android.widget.ImageButton; | ||
import android.widget.LinearLayout; | ||
import android.widget.RelativeLayout; | ||
import android.widget.TextView; | ||
import androidx.appcompat.app.AlertDialog; | ||
import androidx.cardview.widget.CardView; | ||
import androidx.fragment.app.Fragment; | ||
import androidx.fragment.app.FragmentManager; | ||
|
@@ -24,8 +23,12 @@ | |
import androidx.recyclerview.widget.RecyclerView; | ||
import androidx.viewpager.widget.PagerAdapter; | ||
import androidx.viewpager.widget.ViewPager; | ||
import androidx.work.BackoffPolicy; | ||
import androidx.work.Constraints; | ||
import androidx.work.ExistingWorkPolicy; | ||
import androidx.work.NetworkType; | ||
import androidx.work.OneTimeWorkRequest; | ||
import androidx.work.OutOfQuotaPolicy; | ||
import androidx.work.WorkManager; | ||
import butterknife.BindView; | ||
import butterknife.ButterKnife; | ||
|
@@ -35,7 +38,6 @@ | |
import fr.free.nrw.commons.auth.LoginActivity; | ||
import fr.free.nrw.commons.auth.SessionManager; | ||
import fr.free.nrw.commons.contributions.ContributionController; | ||
import fr.free.nrw.commons.contributions.MainActivity; | ||
import fr.free.nrw.commons.filepicker.UploadableFile; | ||
import fr.free.nrw.commons.kvstore.JsonKvStore; | ||
import fr.free.nrw.commons.mwapi.UserClient; | ||
|
@@ -57,6 +59,7 @@ | |
import java.util.Collections; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.concurrent.TimeUnit; | ||
import javax.inject.Inject; | ||
import javax.inject.Named; | ||
import timber.log.Timber; | ||
|
@@ -317,9 +320,18 @@ public void updateTopCardTitle() { | |
|
||
@Override | ||
public void makeUploadRequest() { | ||
Constraints constraints = new Constraints.Builder() | ||
.setRequiredNetworkType(NetworkType.CONNECTED) | ||
.build(); | ||
OneTimeWorkRequest uploadRequest = new OneTimeWorkRequest | ||
.Builder(UploadWorker.class) | ||
.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) | ||
.setConstraints(constraints) | ||
.setBackoffCriteria(BackoffPolicy.LINEAR, 10, TimeUnit.SECONDS) | ||
.build(); | ||
WorkManager.getInstance(getApplicationContext()).enqueueUniqueWork( | ||
UploadWorker.class.getSimpleName(), | ||
ExistingWorkPolicy.APPEND_OR_REPLACE, OneTimeWorkRequest.from(UploadWorker.class)); | ||
ExistingWorkPolicy.APPEND_OR_REPLACE, uploadRequest); | ||
} | ||
|
||
@Override | ||
|
@@ -364,6 +376,37 @@ private void receiveSharedItems() { | |
.getQuantityString(R.plurals.upload_count_title, uploadableFiles.size(), uploadableFiles.size())); | ||
|
||
fragments = new ArrayList<>(); | ||
/* Suggest users to turn battery optimisation off when uploading more than a few files. | ||
That's because we have noticed that many-files uploads have | ||
a much higher probability of failing than uploads with less files. | ||
*/ | ||
if (uploadableFiles.size() > 3 | ||
&& !defaultKvStore.getBoolean("hasAlreadyLaunchedBigMultiupload")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we should show this dialog only for relevant Android versions. The intent itself is only available since API 23 (6+). Kindly check and make sure we only invoke this for the relevant Android versions. |
||
DialogUtil.showAlertDialog( | ||
this, | ||
getString(R.string.unrestricted_battery_mode), | ||
getString(R.string.suggest_unrestricted_mode), | ||
getString(R.string.title_activity_settings), | ||
getString(R.string.cancel), | ||
() -> { | ||
/* Since opening the right settings page might be device dependent, using | ||
https://github.com/WaseemSabir/BatteryPermissionHelper | ||
directly appeared like a promising idea. | ||
However, this simply closed the popup and did not make | ||
the settings page appear on a Pixel as well as a Xiaomi device. | ||
|
||
Used the standard intent instead of using this library as | ||
it shows a list of all the apps on the device and allows users to | ||
turn battery optimisation off. | ||
*/ | ||
Intent batteryOptimisationSettingsIntent = new Intent( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment about the library you tried, explaining how it did not make the setting dialog appear on Pixel nor Xiaomi. Someone may want to revisit that in the future. |
||
Settings.ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS); | ||
startActivity(batteryOptimisationSettingsIntent); | ||
}, | ||
() -> {} | ||
); | ||
defaultKvStore.putBoolean("hasAlreadyLaunchedBigMultiupload", true); | ||
} | ||
for (UploadableFile uploadableFile : uploadableFiles) { | ||
UploadMediaDetailFragment uploadMediaDetailFragment = new UploadMediaDetailFragment(); | ||
uploadMediaDetailFragment.setImageTobeUploaded(uploadableFile, place); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a schema change to the contribution table and requires a bump of the db version number [ref]. Kindly take care of the same.