Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Fetch Diagnosis Keys for current day (contributes to corona-warn-app/cwa-documentation#236) #453

Closed

Conversation

kbobrowski
Copy link
Contributor

Checklist

  • Test your changes as thoroughly as possible before you commit them. Preferably, automate your test by unit/integration tests.
  • If this PR comes from a fork, please Allow edits from maintainers
  • Set a speaking title. Format: {task_name} (closes #{issue_number}). For example: Use logger (closes # 41)
  • Link your Pull Request to an issue (if applicable)
  • Create Work In Progress [WIP] pull requests only if you need clarification or an explicit review before you can continue your work item.
  • Make sure that your PR is not introducing unnecessary reformatting (e.g., introduced by on-save hooks in your IDE)
  • Make sure that your PR does not contain changes in strings.xml (see issue [HELP WANTED][README] Text / Translations  #332)
  • Make sure that your PR does not contain compiled sources (already set by the default .gitignore) and / or binary files

Description

Currently only daily bundles of Diagnosis Keys are fetched, which introduces in worst-case additional 24 hours delay in exposure notification. Fetching hourly Diagnosis Keys decreases this delay and aligns Android implementation with iOS implementation.

Unit test is extended but I have not tested it with live API.

Contributes to corona-warn-app/cwa-documentation#236

@kbobrowski kbobrowski changed the title [WIP] Fetch Diagnosis Keys for current day (Contributes to corona-warn-app/cwa-documentation#236) [WIP] Fetch Diagnosis Keys for current day (contributes to corona-warn-app/cwa-documentation#236) Jun 14, 2020
@kbobrowski kbobrowski marked this pull request as ready for review June 14, 2020 14:20
@kbobrowski kbobrowski requested a review from a team June 14, 2020 14:20
@kbobrowski kbobrowski changed the title [WIP] Fetch Diagnosis Keys for current day (contributes to corona-warn-app/cwa-documentation#236) Fetch Diagnosis Keys for current day (contributes to corona-warn-app/cwa-documentation#236) Jun 16, 2020
@jakobmoellerdev
Copy link
Contributor

Hi! Where does iOS actually have the fetchHours method used? From what I see, the behaviour aligns right now.

@kbobrowski
Copy link
Contributor Author

kbobrowski commented Jun 17, 2020

Hey :) fetchHours is used here on iOS (within fetchDays method). But I'm not iOS dev, so difficult for me to say for certain if in the end hour endpoint is queried.

But I think this is not the main point (alignment with iOS), I wonder why we cannot fetch also hour endpoint? it seems we only fetch days right now, and functionality to fetch hours used to be there. This would greatly decrease delay in providing user with exposure notification. This is a subject of this issue: corona-warn-app/cwa-documentation#236 , if you could clear our doubts over there (especially regarding limits imposed on provideDiagnosisKeys) - it would be great, thanks!

(this issue is quite lengthy, but in the end everything seems to be connected to limits imposed on provideDiagnosisKeys - does it limit number of files passed, or number of calls to the method?, there seems to be discrepancy right now in interpretation of these limits, please check my last comment)

@jakobmoellerdev
Copy link
Contributor

We are preparing a common statement that is applicable for both Apps and should give some clarity on the behaviour

@kbobrowski
Copy link
Contributor Author

@jakobmoellersap thanks for the update, statement applicable to both apps will be of course useful and I'll wait for it then, but in a meantime I think it would still be important to clarify the specific issue of provideDiagnosisKeys - what are the limits here. This is something that community cannot easily test (perhaps using rev-eng tools). I'm curious if you experienced limits on number of files passed or on number of calls to this method - this would help a lot in supporting you with dev work here

@kbobrowski kbobrowski marked this pull request as draft June 22, 2020 18:24
@kbobrowski
Copy link
Contributor Author

I've experimented with exposure notification client captured from Java heap and fully understand now limitations of current provideDiagnosisKeys, it will not accept multiple files which have batch_size set to 1. Google should allow soon multiple batches per request (https://github.com/corona-warn-app/cwa-documentation/issues/236#issuecomment-647618727), perhaps then it would be possible to include hourly batches without making deep changes to current architecture

@jakobmoellerdev
Copy link
Contributor

As discussed, this will most likely be revisited with the next major Google API Update and or a our architecture alignment regarding background jobs. Thus I will close the PR as it would not make sense to introduce changes to the logic that is subject to change right now.

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

Successfully merging this pull request may close these issues.

4 participants