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

[mqtt] Fix availability topics subscription after Brige Restart #9851

Merged
merged 1 commit into from
Dec 11, 2021

Conversation

albrechtf
Copy link
Contributor

This is a Patch Proposal for #9850. It is the "easy" variant of the fix; the major design flaws in classes AbstractMQTTThingHandler and GenericMQTTThingHandler (especially regarding availability topics) are not addressed.

A Unit Test is included.

@albrechtf albrechtf force-pushed the fix_9850_mqtt_avail_topics branch from 1589787 to fd14bca Compare January 17, 2021 14:19
@albrechtf albrechtf force-pushed the fix_9850_mqtt_avail_topics branch from fd14bca to a956bce Compare January 17, 2021 14:26
@albrechtf
Copy link
Contributor Author

CI build fails (stops) after 1 hour. Appreciate any support how to "fix" that :-)

@wborn wborn added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 18, 2021
@Hilbrand Hilbrand added the bug An unexpected problem or unintended behavior of an add-on label Jan 20, 2021
} else {
clearAllAvailabilityTopics();
}
initializeAvailabilityTopicsFromConfig();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell, due to your changes in start above, topic subscription will occur automatically as part of the call to super.initialize(). So you don't need to call initializeAvailabilityTopicsFromConfig() here at all. In fact it might be better if you don't since that way config errors will prevent further initialization, as they should.

You should also make changes in AbstractMQTTThingHandler.bridgeStatusChanged to make sure that the handler isn't offline due to config errors before it tries initializing a connection.

Comment on lines +195 to 196
start(connection).get(subscribeTimeout, TimeUnit.MILLISECONDS);
} catch (InterruptedException | ExecutionException | TimeoutException ignored) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if you made sure that the offline status would include the exception message, just like it did before.

@fwolter fwolter added the awaiting feedback Awaiting feedback from the pull request author label Feb 23, 2021
@fwolter
Copy link
Member

fwolter commented Dec 11, 2021

@albrechtf What's the state of this PR?

@albrechtf
Copy link
Contributor Author

@fwolter The state is that my PR was not good enough for the maintainers, and I currently don't have time to adjust it according to their comments. Feel free to provide additional changes / patches so it may be good enough for merging, that would be very much appreciated.

@fwolter fwolter added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Dec 11, 2021
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review comments are valid. In the meantime we lowered our quality standards a bit to accommodate the increasing number of PRs. If you find the time in the future to address the comments, please go ahead.

@fwolter fwolter merged commit 13ca0d5 into openhab:main Dec 11, 2021
@fwolter fwolter removed the awaiting feedback Awaiting feedback from the pull request author label Dec 11, 2021
@fwolter fwolter added this to the 3.2 milestone Dec 11, 2021
NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Dec 30, 2021
mischmidt83 pushed a commit to mischmidt83/openhab-addons that referenced this pull request Jan 9, 2022
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Jan 28, 2022
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants