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

[DPM-2282] Unable to create service net.gotev.uploadservice.UploadService: java.lang.IllegalArgumentException #12

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

angel-rs
Copy link

@angel-rs angel-rs commented Apr 1, 2024

Objective *

  • Use WorkManager API
  • Remove Unable to create service net.gotev.uploadservice.UploadService: java.lang.IllegalArgumentException error

Details and Context *

Changes made:

  • Rename android package com.vydia.RNUploader to com.sitemate.uploader
  • Increase android connection timeouts
  • In Android use the WorkManager api to perform the uploads inside of a job. This should allow the uploads to be more reliable & for there to not be errors/crashes when putting the app in background/locking the device.

DPM-2282

https://www.loom.com/share/cd0b3d1c030c4dda9854e599d5cfe4b2

Pre-Submission Checklist *

  • I have reviewed all the changes one final time before opening this PR.

Tests

  • I have added automated tests for all new code introduced in this PR.
  • I have not added automated tests, because... (explain below)

Risks and uncertainty (optional)

  • This PR doesn't introduce any new risks or uncertainties.
  • This PR introduces new risks or uncertainties, because... (explain below)

Additional Information

  • Feedback Ladder Reminder

    • Mountain: Blocker & immediate action needed.
    • Boulder: Blocker & requires action
    • Pebble: Non-blocking, but requires future action (in either this or a future PR)
    • Sand: Non-blocking, but is generally best practice
    • Dust: Non-blocking comment
  • Pull Request Standards

@angel-rs angel-rs changed the title Feat android/dpm 2282 migrate to work api [DPM-2282] Unable to create service net.gotev.uploadservice.UploadService: java.lang.IllegalArgumentException Apr 1, 2024
@angel-rs angel-rs self-assigned this Apr 1, 2024
@timjbray
Copy link
Member

timjbray commented Apr 4, 2024

@angel-rs looks good, couple q's

  • where did most this code come from - you said another fork? how reliable/tests is it?
  • what's the best testing plan?
    -- is there a way we can give ourselves a level of confidence this
    -- how do we test all the changes?
  • what's the safest release plan? nervous to just release this as once to all customers 😟
    -- idea would be to release this in isolation as a hot fix and fully control the rollout.

@angel-rs
Copy link
Author

angel-rs commented Apr 4, 2024

@timjbray Really good questions! cc @site-mate/mobile-team

where did most this code come from - you said another fork?

Yes. Used this tool to get the most popular forks of the original lib & Skimmed through the commits they did on top of the original lib. Found that the appfolio's fork is the most popular among the forks & uses the WorkManager api, which is what I was after.

how reliable/tests is it?

Should be as reliable as the original library. IMO should be better as it now uses the WorkManager api to schedule the uploads & it also increases the config for network timeouts
We'll learn more about it when we do proper testing on it.

what's the best testing plan?

I've copied + extended the tests we performed when first released background uploading (They are in the QA notes section in the ticket):

Screenshot 2024-04-04 at 10 52 31 AM

is there a way we can give ourselves a level of confidence this
how do we test all the changes?

The most reliable way would be with manual testing with as many scenarios as possible, with multiple devices, etc.
Similarly with how we tested react-native-vision-camera
Not perfect, but it's the only way I can think of building real confidence over the package.

what's the safest release plan? nervous to just release this as once to all customers 😟

Agree. Doesn't make sense to include in DPM 24.1
Perhaps we can hold it off. As there's no harm in doing so. There's no other active work in this repo & the DPM changes are trivial.

idea would be to release this in isolation as a hot fix and fully control the rollout.

Like that idea.
Only concern is that if we need to rollback, we'd need another hotfix (which we could have prepared ahead of time?)
TOL: we could do the upgrade first in SMM? As the app is simpler there. Less risk for power users which are in DPM?

We can't really roll this out dynamically easily as it's a native library.
One idea would be to have 2 versions of react-native-background-upload, e.g:

react-native-background-upload -> existing version in DPM
react-native-background-upload2 -> this new version (the package name was changed, so in theory it shouldn't affect?

Then implement something like:

// uploader.ts
import Upload1 from 'react-native-background-upload'
import Upload2 from 'react-native-background-upload2'

let Upload = Upload1 // start with the current one.

// on app boot, fetch some public bucket file that stores a boolean if we want to enable the new uploader
// this way we would have a 'kill switch' of this new uploader implementation.
fetch(...).then((res) => {
  if (res.enableNewUploader) {
    Upload = Upload2
  }
})

export { Upload } 

But not 100% sure it would work, would need to put the work & test.
Will be putting this in the shelf while working on the Person Field RFC (needed for next week)
Can then resume this work & test the above.

@timjbray
Copy link
Member

timjbray commented Apr 9, 2024

@angel-rs solid answers.

Having both bg uploading libraries might be more risk that it solves. Let's release as a hot fix slowly after a major DPM release:

  • it's only android
  • we have full control of the rollout
  • fast and easy to revert

I'm slightly concerned this library is responsible for some of our ANR's. So moving to the work manager might need to be fast tracked. Still could releasing straight after DPM 24.1, as long as we can get it out soon.

cc @site-mate/mobile-team

@angel-rs
Copy link
Author

@timjbray Sounds like a plan.

Once DPM 24.1 passes regressions - I can then finish the work here & move to UAT.

But then we'll need to decide when it goes live. Perhaps once DPM 24.1 is rolled out at 100%, or when we force upgrade to 24.1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants