-
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
Upload treatments to multiple Nightscout sites #3630
base: master
Are you sure you want to change the base?
Conversation
I created two new free Nightscout sites using our Google Cloud scheme. I used a test phone with xDrip as a follower of my active xDrip on my main phone. First, before making any changes, I added lots of logs to make the behavior of xDrip on the test phone obvious. After adding the changes in this PR to xDrip, treatments were uploaded to both sites. |
I am not 100% sure. But, it may be that the queue has a wider scope than it should have. Therefore, when it is completed inside a member of the loop, when the next member of the loop attempts to recreate the same queue, it fails. So, I am sure there are other ways to fix this issue. I have fixed it by making the completion conditional. So, when the second member of the loop attempts to create the queue and it sees that it already exists, I suppose it does not recreate it. Please tell me how I can help to make the review easier. Would it help if I undo the auto formatting? |
Edit: @jamorham Would you please have a look and let me know how I can improve it if needed? |
13029b9
to
8ed51e7
Compare
@jamorham Would you please review this when you get a chance? If testing by others would help, I can release a test release and ask a few individuals to test and report. |
The current logic is intended that if any site upload succeeds then the queue will be marked as completed. This is because multiple sites was added later and the queue entry isn't able to differentiate which site uploads were successful to. Looking through the code it looks like this fails for treatment uploads because the function being called postTreatments() attempts to read the pending queue entries which were already marked as completed if the first site upload succeeds. I think the changes you propose will create the opposite situation, so that if site 1 is up and site 2 is down then items will never leave the queue and will be endlessly uploaded to site 1. The proper fix for this would be queues per site but the architecture doesn't lend itself very well to that at all and if we were going to do that we should also start properly using the nightscout sdk which would also likely improve overall handling of this. In the short term, I think the fix which preserves the original behavior of just ignoring failures to sites which fail so long as one upload succeeds can quite easily be preserved by simply moving the line of code which retrieves the queue list:
Move this to the parent caller outside of the loop per site and then pass it in as a variable. The fact that entries will be marked as completed I don't think will affect their upload then, it is likely only where they are filtered out by You'll probably have to move that code two levels out to I believe that should work and is a safer solution that you have proposed in this PR. |
Thank you very much. I had overlooked the intent of the queue. Thanks so much for teaching me that here. |
@jamorham I repeated my test and disabled one of the two sites. xDrip showed an error in red on the system status page while readings were only uploaded to the active site. Would doing this the proper way be extremely complicated so that you feel you must do it yourself? This is really bad that if someone has two sites entered, the queue will not be maintained if only one site is active. |
Fixes: #266
xDrip only uploads treatments to the first site if the user enters multiple URLs separated by space as the base URL.
I have confirmed this is because during the very first run of the for loop going through the multiple sites, the treatment upload queue is completed (up.completed).
I have added a counter to detect the last run and made the completed commands conditional on being in the last run of the loop.
My tests show that treatments are now uploaded to multiple sites.
I did the auto format because the indentations are very confusing.
I had unintentionally left some test logs in a previous PR. I have removed those.