From 9213863f0ff848d628dc43b0916408e8e5694504 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Wed, 7 Dec 2022 16:54:59 +0100 Subject: [PATCH 1/4] in OnRemoteOperationFinish: rename callerWrapper into syncWrapper --- .../e/drive/services/SynchronizationService.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java index ed0f2ab9..df12a597 100644 --- a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java +++ b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java @@ -247,18 +247,19 @@ public class SynchronizationService extends Service implements OnRemoteOperation @Override public void onRemoteOperationFinish(RemoteOperation callerOperation, RemoteOperationResult result) { Timber.i("onRemoteOperationFinish()"); - SyncWrapper callerWrapper = null; + SyncWrapper syncWrapper = null; + for (Map.Entry keyValue : startedSync.entrySet()) { if (keyValue.getValue().getRemoteOperation().equals(callerOperation)) { - callerWrapper = keyValue.getValue(); - callerWrapper.setRunning(false); - startWorker(keyValue.getKey()); + syncWrapper = keyValue.getValue(); + syncWrapper.setRunning(false); + startWorker(keyValue.getKey()); //todo: maybe move that to end of method break; } } - if (callerWrapper != null) { - updateFailureCounter(callerWrapper.getRequest(), result.isSuccess()); + if (syncWrapper != null) { + updateFailureCounter(syncWrapper.getRequest(), result.isSuccess()); } if (callerOperation instanceof RemoveFileOperation) { -- GitLab From 7a77fe96522002e271ca6b1b2080f50be8dbb238 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Wed, 7 Dec 2022 16:56:19 +0100 Subject: [PATCH 2/4] in OnRemoteOperationFinish: rename callerOperation into operation --- .../services/SynchronizationService.java | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java index df12a597..a2169e6e 100644 --- a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java +++ b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java @@ -245,12 +245,12 @@ public class SynchronizationService extends Service implements OnRemoteOperation } @Override - public void onRemoteOperationFinish(RemoteOperation callerOperation, RemoteOperationResult result) { + public void onRemoteOperationFinish(RemoteOperation operation, RemoteOperationResult result) { Timber.i("onRemoteOperationFinish()"); SyncWrapper syncWrapper = null; for (Map.Entry keyValue : startedSync.entrySet()) { - if (keyValue.getValue().getRemoteOperation().equals(callerOperation)) { + if (keyValue.getValue().getRemoteOperation().equals(operation)) { syncWrapper = keyValue.getValue(); syncWrapper.setRunning(false); startWorker(keyValue.getKey()); //todo: maybe move that to end of method @@ -262,13 +262,13 @@ public class SynchronizationService extends Service implements OnRemoteOperation updateFailureCounter(syncWrapper.getRequest(), result.isSuccess()); } - if (callerOperation instanceof RemoveFileOperation) { + if (operation instanceof RemoveFileOperation) { if ( result.isSuccess() ) { - DbHelper.manageSyncedFileStateDB( ( ( RemoveFileOperation ) callerOperation ).getSyncedFileState(), + DbHelper.manageSyncedFileStateDB( ( ( RemoveFileOperation ) operation ).getSyncedFileState(), "DELETE", this); } } else { - final String operationClassName = callerOperation.getClass().getSimpleName(); + final String operationClassName = operation.getClass().getSimpleName(); switch (result.getCode()) { case OK: Timber.d("%s Succeed", operationClassName); @@ -281,18 +281,18 @@ public class SynchronizationService extends Service implements OnRemoteOperation Timber.e("%s => invalid_overwrite :\n remote file and local file doesn't have the same size", operationClassName); break; case UNKNOWN_ERROR: - if (callerOperation instanceof UploadFileOperation) { - final int rowAffected = DbHelper.forceFoldertoBeRescan(((UploadFileOperation) callerOperation).getSyncedState().getId(), getApplicationContext()); + if (operation instanceof UploadFileOperation) { + final int rowAffected = DbHelper.forceFoldertoBeRescan(((UploadFileOperation) operation).getSyncedState().getId(), getApplicationContext()); Timber.e("Upload failed for unknown reason.\n Force folder to be rescan next time (row affected) : %s", rowAffected); - } else if (callerOperation instanceof DownloadFileOperation) { + } else if (operation instanceof DownloadFileOperation) { Timber.e("Download: Unknown_error : failed"); } break; case FORBIDDEN: - if (callerOperation instanceof UploadFileOperation) { - final int rowAffected = DbHelper.forceFoldertoBeRescan(((UploadFileOperation) callerOperation).getSyncedState().getId(), getApplicationContext()); + if (operation instanceof UploadFileOperation) { + final int rowAffected = DbHelper.forceFoldertoBeRescan(((UploadFileOperation) operation).getSyncedState().getId(), getApplicationContext()); Timber.e("Upload: Forbidden : Can't get syncedFileState, no remote path defined. Force folder to be rescan next time (row affected) : %s", rowAffected); - } else if (callerOperation instanceof DownloadFileOperation) { + } else if (operation instanceof DownloadFileOperation) { Timber.e("Download : Forbidden: Can't get syncedFileState, no local path defined"); } break; -- GitLab From 6123d4639d4bb011a624a2f76c9acd05891bba1a Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Wed, 7 Dec 2022 17:33:25 +0100 Subject: [PATCH 3/4] Rewrite onRemoteOperationFinish, for better logging and performance --- .../services/SynchronizationService.java | 69 ++++++++++--------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java index a2169e6e..4076be69 100644 --- a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java +++ b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java @@ -7,6 +7,7 @@ */ package foundation.e.drive.services; +import static com.owncloud.android.lib.common.operations.RemoteOperationResult.ResultCode.OK; import static foundation.e.drive.operations.UploadFileOperation.FILE_SIZE_FLOOR_FOR_CHUNKED; import android.accounts.Account; @@ -33,6 +34,8 @@ import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentLinkedDeque; +import javax.xml.transform.Result; + import foundation.e.drive.database.DbHelper; import foundation.e.drive.database.FailedSyncPrefsManager; import foundation.e.drive.models.SyncRequest; @@ -246,70 +249,70 @@ public class SynchronizationService extends Service implements OnRemoteOperation @Override public void onRemoteOperationFinish(RemoteOperation operation, RemoteOperationResult result) { - Timber.i("onRemoteOperationFinish()"); - SyncWrapper syncWrapper = null; + final boolean succeed = result.isSuccess() || result.getCode() == OK; + final String operationClassName = operation.getClass().getSimpleName(); - for (Map.Entry keyValue : startedSync.entrySet()) { - if (keyValue.getValue().getRemoteOperation().equals(operation)) { - syncWrapper = keyValue.getValue(); - syncWrapper.setRunning(false); - startWorker(keyValue.getKey()); //todo: maybe move that to end of method - break; - } - } - - if (syncWrapper != null) { - updateFailureCounter(syncWrapper.getRequest(), result.isSuccess()); - } - - if (operation instanceof RemoveFileOperation) { - if ( result.isSuccess() ) { - DbHelper.manageSyncedFileStateDB( ( ( RemoveFileOperation ) operation ).getSyncedFileState(), + String statusLogMsg = ""; + if (succeed) { + if (operation instanceof RemoveFileOperation) { + DbHelper.manageSyncedFileStateDB( ((RemoveFileOperation) operation).getSyncedFileState(), "DELETE", this); } + statusLogMsg = "succeed"; } else { - final String operationClassName = operation.getClass().getSimpleName(); switch (result.getCode()) { - case OK: - Timber.d("%s Succeed", operationClassName); - break; case SYNC_CONFLICT: //Case specific to UploadFileOperation - Timber.e("%s : Sync_conflict : File is already up to date", operationClassName); + statusLogMsg = "failed: File is already up to date (Sync_conflict)"; break; case INVALID_OVERWRITE: - Timber.e("%s => invalid_overwrite :\n remote file and local file doesn't have the same size", operationClassName); + statusLogMsg = "failed: invalid_overwrite, remote and local size are different"; break; case UNKNOWN_ERROR: if (operation instanceof UploadFileOperation) { final int rowAffected = DbHelper.forceFoldertoBeRescan(((UploadFileOperation) operation).getSyncedState().getId(), getApplicationContext()); - Timber.e("Upload failed for unknown reason.\n Force folder to be rescan next time (row affected) : %s", rowAffected); - } else if (operation instanceof DownloadFileOperation) { - Timber.e("Download: Unknown_error : failed"); + Timber.d("Force folder to be rescan next time (row affected) : %s", rowAffected); } + statusLogMsg = "failed : unknown error"; break; case FORBIDDEN: if (operation instanceof UploadFileOperation) { final int rowAffected = DbHelper.forceFoldertoBeRescan(((UploadFileOperation) operation).getSyncedState().getId(), getApplicationContext()); - Timber.e("Upload: Forbidden : Can't get syncedFileState, no remote path defined. Force folder to be rescan next time (row affected) : %s", rowAffected); - } else if (operation instanceof DownloadFileOperation) { - Timber.e("Download : Forbidden: Can't get syncedFileState, no local path defined"); + Timber.d("Force folder to be rescan next time (row affected) : %s", rowAffected); } + statusLogMsg ="failed: Forbidden, Can't get syncedFileState, no local path defined"; break; case QUOTA_EXCEEDED: //Case specific to UploadFileOperation - Timber.w("Quota_EXCEEDED"); + statusLogMsg ="failed: Quota_EXCEEDED"; break; case FILE_NOT_FOUND: //Case specific to DownloadFileOperation - Timber.e("%s : File_not_found: File not found after download", operationClassName); + statusLogMsg ="failed: file_not_found after download"; break; case ETAG_UNCHANGED: //Case specific to DownloadFileOperation - Timber.e("%s : Sync_conflict: File is already up to date", operationClassName); + statusLogMsg = "failed: Etag unchanged, File is already up to date (Sync_conflict)"; break; } } + + for (Map.Entry keyValue : startedSync.entrySet()) { + if (keyValue.getValue().getRemoteOperation().equals(operation)) { + final SyncWrapper syncWrapper = keyValue.getValue(); + if (syncWrapper == null) return; + + updateFailureCounter(syncWrapper.getRequest(), succeed); + syncWrapper.setRunning(false); + + Timber.w("%s of %s %s", operationClassName, + syncWrapper.getRequest().getSyncedFileState().getLocalPath(), + statusLogMsg); + + startWorker(keyValue.getKey()); + return; + } + } } private void updateFailureCounter(SyncRequest request, boolean success) { -- GitLab From fff7a23339187b2d2964d153e38a40b8089d37b5 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Thu, 8 Dec 2022 10:32:35 +0100 Subject: [PATCH 4/4] Isolate code from 'onRemoteOperationFinish' into a dedicated method 'createRemoteOperationErrorLogMsg(ResultCode)' --- .../services/SynchronizationService.java | 81 ++++++++++--------- 1 file changed, 44 insertions(+), 37 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java index 4076be69..6abbf1f3 100644 --- a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java +++ b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java @@ -7,7 +7,9 @@ */ package foundation.e.drive.services; +import static com.owncloud.android.lib.common.operations.RemoteOperationResult.ResultCode.FORBIDDEN; import static com.owncloud.android.lib.common.operations.RemoteOperationResult.ResultCode.OK; +import static com.owncloud.android.lib.common.operations.RemoteOperationResult.ResultCode.UNKNOWN_ERROR; import static foundation.e.drive.operations.UploadFileOperation.FILE_SIZE_FLOOR_FOR_CHUNKED; import android.accounts.Account; @@ -34,13 +36,10 @@ import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentLinkedDeque; -import javax.xml.transform.Result; - import foundation.e.drive.database.DbHelper; import foundation.e.drive.database.FailedSyncPrefsManager; import foundation.e.drive.models.SyncRequest; import foundation.e.drive.models.SyncWrapper; -import foundation.e.drive.operations.DownloadFileOperation; import foundation.e.drive.operations.RemoveFileOperation; import foundation.e.drive.operations.UploadFileOperation; import foundation.e.drive.utils.AppConstants; @@ -201,8 +200,10 @@ public class SynchronizationService extends Service implements OnRemoteOperation } private void startWorker(Integer threadIndex){ + Timber.v("startWorker: remaining request: %s", syncRequestQueue.size()); if (!canStart(threadIndex)) return; + final SyncRequest request = this.syncRequestQueue.poll(); //return null if empty if (request == null || request.getSyncedFileState() == null @@ -260,40 +261,11 @@ public class SynchronizationService extends Service implements OnRemoteOperation } statusLogMsg = "succeed"; } else { - switch (result.getCode()) { - case SYNC_CONFLICT: - //Case specific to UploadFileOperation - statusLogMsg = "failed: File is already up to date (Sync_conflict)"; - break; - case INVALID_OVERWRITE: - statusLogMsg = "failed: invalid_overwrite, remote and local size are different"; - break; - case UNKNOWN_ERROR: - if (operation instanceof UploadFileOperation) { - final int rowAffected = DbHelper.forceFoldertoBeRescan(((UploadFileOperation) operation).getSyncedState().getId(), getApplicationContext()); - Timber.d("Force folder to be rescan next time (row affected) : %s", rowAffected); - } - statusLogMsg = "failed : unknown error"; - break; - case FORBIDDEN: - if (operation instanceof UploadFileOperation) { - final int rowAffected = DbHelper.forceFoldertoBeRescan(((UploadFileOperation) operation).getSyncedState().getId(), getApplicationContext()); - Timber.d("Force folder to be rescan next time (row affected) : %s", rowAffected); - } - statusLogMsg ="failed: Forbidden, Can't get syncedFileState, no local path defined"; - break; - case QUOTA_EXCEEDED: - //Case specific to UploadFileOperation - statusLogMsg ="failed: Quota_EXCEEDED"; - break; - case FILE_NOT_FOUND: - //Case specific to DownloadFileOperation - statusLogMsg ="failed: file_not_found after download"; - break; - case ETAG_UNCHANGED: - //Case specific to DownloadFileOperation - statusLogMsg = "failed: Etag unchanged, File is already up to date (Sync_conflict)"; - break; + statusLogMsg = createRemoteOperationErrorLogMsg(result.getCode()); + if( operation instanceof UploadFileOperation && + (result.getCode().equals(UNKNOWN_ERROR) || result.getCode().equals(FORBIDDEN))) { + final int rowAffected = DbHelper.forceFoldertoBeRescan(((UploadFileOperation) operation).getSyncedState().getId(), getApplicationContext()); + Timber.d("Force folder to be rescan next time (row affected) : %s", rowAffected); } } @@ -315,6 +287,41 @@ public class SynchronizationService extends Service implements OnRemoteOperation } } + private String createRemoteOperationErrorLogMsg(RemoteOperationResult.ResultCode resultCode) { + final String result; + switch (resultCode) { + case SYNC_CONFLICT: + //Case specific to UploadFileOperation + result = "failed: File is already up to date (Sync_conflict)"; + break; + case INVALID_OVERWRITE: + result = "failed: invalid_overwrite, remote and local size are different"; + break; + case UNKNOWN_ERROR: + result = "failed : unknown error"; + break; + case FORBIDDEN: + result = "failed: Forbidden, Can't get syncedFileState, no local path defined"; + break; + case QUOTA_EXCEEDED: + //Case specific to UploadFileOperation + result = "failed: Quota_EXCEEDED"; + break; + case FILE_NOT_FOUND: + //Case specific to DownloadFileOperation + result = "failed: file_not_found after download"; + break; + case ETAG_UNCHANGED: + //Case specific to DownloadFileOperation + result = "failed: Etag unchanged, File is already up to date (Sync_conflict)"; + break; + default: + result = "Invalid result code"; + break; + } + return result; + } + private void updateFailureCounter(SyncRequest request, boolean success) { final FailedSyncPrefsManager failedPref = FailedSyncPrefsManager.getInstance(getApplicationContext()); final int fileStateId = request.getSyncedFileState().getId(); -- GitLab