Migrate jobs to new user model #5447
Conversation
ezaquarii
force-pushed
the
ezaquarii/migrate-jobs-to-new-user-model
branch
from
9301750 to
0e0be92
Compare
ezaquarii
changed the title
Codecov Report
@@ Coverage Diff @@
## master #5447 +/- ##
============================================
- Coverage 20.93% 20.92% -0.01%
Complexity 3 3
============================================
Files 398 398
Lines 33324 33353 +29
Branches 4674 4678 +4
============================================
+ Hits 6975 6978 +3
- Misses 25219 25245 +26
Partials 1130 1130
|
Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>
ezaquarii
force-pushed
the
ezaquarii/migrate-jobs-to-new-user-model
branch
from
0e0be92 to
0afc264
Compare
ezaquarii
changed the title
| private final UploadsStorageManager uploadsStorageManager; | ||
| private final UserAccountManager userAccountManager; | ||
| private final Clock clock; | ||
| private final EventBus eventBus; |
There was a problem hiding this comment.
I know that this was used before, so I am totally fine to re-use this.
But as our approach as to get rid of 3rd party libs, we should think about removing EventBus and evernote Job scheduling, but rely on WorkManager and
[https://developer.android.com/topic/libraries/architecture/workmanager/how-to/states-and-observation#observing](observing your work)
But let us do this in a following PR, please :-)
There was a problem hiding this comment.
I understand the evernote thing, but why is it that we hate eventbus these days? It's a relatively established and long-living project that won't disappear tomorrow.
There was a problem hiding this comment.
@tobiasKaminsky I didn't add it - I just extracted a dependency that was explicitly created in a job constructor.
@mario Good question!
EventBus is not a bad piece of engineering in itself. However, how it's being used in practice is another story and mileage will vary.
I wouldn't say I personally hate it. Event bus design pattern has it's place. I think that - as usual with engineering concepts - some people observed abuses and spoke loud about them. Some were more mature at verbalizing the issues, some - maybe pursuing social maedia traction - just vented out anger calling ppl names.
If we ask ourselves "why" in a serious manner, I can find few issues with event bus pattern in general, in no relation to Nextcloud app:
- it's very temping to inroduce hidden tight coupling between componets (no direct source-level dependency, but dependencies caused by complex event patterns)
- cascade of event posts makes callstack analysis challenging; it becomes very tricky if events are sent bi-directionally between many actors; this is what makes contribues seriously to microservices debugging difficulty and market is raging with fancy distributed-log-as-a-service solutions
- event storms :-)
- upredictable and untraceable code paths - your component can be called from many places
Those issues don't necessarily make event bus an anti-pattern, but migh influence a decision of using it or not. It can surely save you some typing, but typing is not a bottleneck in more complex systems. Also, at certain level of complexity, team/company social considerations cannot be dismissed lightly and more involving patterns, but requiring less engineering discipline might be preferred over more convenient ones, but requiring more informed or governed approach.
BTW, we have LocalBroadcastManager that can serve this role as well. The drawback of LMB is that it requires Android platform in tests, so I wouldn't be so sure at all that we are anxious to remove EventBus. :-)
There was a problem hiding this comment.
I am aware of "people" complaining and the bad patterns surrounding EventBus, some of which are also inside Nextcloud (not the least, some written by me). Nevertheless, I really appreciate you taking the time to explain your thoughts in detail. Thank you once again for being awesome!
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
|
@ezaquarii I took the liberty to change the last cancelContactBackupJobForAccount to use also User Otherwis fine and approved 🎉 |
|
Issues
======
+ Solved 1
- Added 4
Complexity increasing per file
==============================
- src/main/java/com/owncloud/android/jobs/AccountRemovalJob.java 2
See the complete overview on Codacy |
Thanks! I'm done here for now, so let's ship it. |
|
@tobiasKaminsky Drone hangs and it's not the first time it does it.
Would it be possible to set a timeout or - even better - timeout + automatic restart? |
|
IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12701-IT |
|
IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12702-IT |
There is a timeout after 30min, not sure why this sometimes is ignored. |
|
I restarted, and then merge :-) |
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/12709.apk |
Codacy347Lint
SpotBugs (new)
SpotBugs (master)
|
|
IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12709-IT |
delete-merged-branch
bot
deleted the
ezaquarii/migrate-jobs-to-new-user-model
branch
2b26a51 Merge remote-tracking branch 'origin/master' into dev 4616b0d Merge pull request #5447 from nextcloud/ezaquarii/migrate-jobs-to-new-user-model 20b910a Merge pull request #5442 from nextcloud/ezaquarii/fix-crash-on-text-file-creation 0977c37 Fix crash during text file creation 5c74f2b [tx-robot] updated from transifex 27fabe7 [tx-robot] updated from transifex 6029548 daily dev 20200215
Jobs are migrated to new user model.
Some of the APIs that jobs use are not yet user-ready - we use
toPlatformAccount. As ususal, the plan is to scoop those cases when we refactor those dependencies.There should be no changes in logic - this is pretty mechanical change.