-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[netatmo] API limit reached handling #16489
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that was fast! Thanks for taking care. I will take a closer look at my logs, your fixes and also give it a run on my production system.
...ing.netatmo/src/main/java/org/openhab/binding/netatmo/internal/handler/ApiBridgeHandler.java
Outdated
Show resolved
Hide resolved
You were lucky enough that I had some spare time today - I did not launch eclipse since around 3 month. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ported your changes to 4.1.x and put it on my production system. All Things went offline pretty quickly - the API bridge with: "Request timed out - will attempt reconnection later".
Does it work well for you?
...ing.netatmo/src/main/java/org/openhab/binding/netatmo/internal/handler/ApiBridgeHandler.java
Outdated
Show resolved
Hide resolved
07c499b
to
f420e73
Compare
@jlaur : if you can test on this base. I've reached something that seems pretty strong on heavy workload and normally solved the ghost process issue. |
Thanks! I've backported your changes to 4.1.x and just put the binding on my production system. It started without issues, but quickly after I noticed this:
and:
I'm not sure if the exception is related to the undisposed task from the previous version. In that case, it can be ignored. I have now restarted openHAB and will let the binding run for some time, and I will also start analyzing your changes in relation to the experienced issues and see if I can figure out which changes addresses which issues. 🙂 |
...ing.netatmo/src/main/java/org/openhab/binding/netatmo/internal/handler/ApiBridgeHandler.java
Show resolved
Hide resolved
@clinique - strangely, if I
Also, I do not see anything logged regarding probing, i.e. "Data validity period identified to be" nor "Data validity period not yet found, data timestamp unchanged". I cannot rule out that I made some mistake when backporting the changes, although I went over everything a few times. Do you get different results? |
Yes, I see the same. Looking at the thing definition, it has a refreshInterval configuration : |
I see you removed the default value of 180 seconds. But should it even have this configuration parameter? According to the documentation this only applies to the |
Yes, it's only a quick fix. A change has been introduced here that created this issue. I'll take some to think of it and partially revert or correct PR #16492 |
@jlaur : can you give a test or your feed-back on this ? I rebased with last merges. |
Yes, I've had it running in production since Thursday after #16546 was merged. I'm currently running some tests after your latest commit today: Additional module:
Main module:
Restart binding:
Simulate API responses:
Currently I'm checking the results of main module getting back online which doesn't seem to work. For the last three tests I have provided a new console command for simulating exceptions and HTTP status codes. I will keep you updated on the results. |
...src/main/java/org/openhab/binding/netatmo/internal/handler/capability/RefreshCapability.java
Outdated
Show resolved
Hide resolved
On first OAuth failure there are two status updates:
Retry is correctly scheduled:
After second failure:
and it doesn't recover after that. |
Signed-off-by: Jacob Laursen <[email protected]>
In case you are interested, see 62ae29d. |
@clinique - why did you close this PR? |
I don't know. Probably a mistake, I made some cleaning in my repo...sorry. |
@clinique - I'm now back to this. Can I ask how you tested this? I'm asking because I have applied my "console error simulator", and I immediately ran into an issue when being authenticated already, but then hitting an HTTP error code 429:
(and then everything came back online) Then problem I see here is that we seem not to be protected at all from worsening after MAXIMUM_USAGE_REACHED or HTTP error code 429, since the child handlers are still permitted to call the API, and are actively doing so. Looking at the code, I think the "API limiting" is only for the corner case of missing authentication or request errors? Maybe I'm testing it the wrong way or misunderstood the intentions. EDIT: It seems that after this occurred, API calls are still made periodically, but nothing is updated anymore. Example:
|
...ing.netatmo/src/main/java/org/openhab/binding/netatmo/internal/handler/ApiBridgeHandler.java
Outdated
Show resolved
Hide resolved
I'll be back on this one very soon. I'm currently focused on another PR I had open since a while. |
a189039
to
09cd57e
Compare
@jlaur : If you are ready to reactivate your console error simulator, I'm ready to give this a new push. |
I will give it another look within the next week. In the meantime you could also give it a spin yourself: 62ae29d. 😉 What is the current state of the PR, perhaps you can give a short update in relation to this comment? |
5747ec8
to
cfda820
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not yet tested it, did you try emulating some error conditions? Also it seems like this comment is not yet addressed? I think when we hit 429, we should make sure we don't call the service at all for some time.
...ing.netatmo/src/main/java/org/openhab/binding/netatmo/internal/handler/ApiBridgeHandler.java
Show resolved
Hide resolved
...ing.netatmo/src/main/java/org/openhab/binding/netatmo/internal/handler/ApiBridgeHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: clinique <[email protected]>
Signed-off-by: clinique <[email protected]>
Signed-off-by: clinique <[email protected]>
Signed-off-by: clinique <[email protected]>
Rebased Signed-off-by: clinique <[email protected]> Signed-off-by: [email protected] <[email protected]>
Rebased Reintroducing TOO_MANY_REQUESTS Signed-off-by: clinique <[email protected]> Signed-off-by: [email protected] <[email protected]>
Rebased Reintroducing TOO_MANY_REQUESTS Rebased Signed-off-by: clinique <[email protected]> Signed-off-by: [email protected] <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
When we have a 429 the bridge will be put offline (then all childs) and a new connection will be tried in 3600s. Is this not enough or am I missing something in your comment ? |
Improve handling of interrupted request by alllowing retries Rebased Reintroducing TOO_MANY_REQUESTS Signed-off-by: clinique <[email protected]> Signed-off-by: [email protected] <[email protected]> Signed-off-by: Gaël L'hopital <[email protected]>
cfda820
to
baee242
Compare
It's all in the referenced comment. I don't see anything related changed since making that comment, so I just assumed it would still be an issue. I need to get my focus back to this topic and run the tests again. |
Distinct timing (60 minutes) when API LIMIT REACHED has been issued.
Partially resolves issue #16485