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

Fix reloading channel config changes in .items file #4075

Merged
merged 3 commits into from
Feb 4, 2024

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Jan 31, 2024

Apply channel config changes in .items file

Changes in channel config weren't applied because ItemChannelLink.equals() include the link configurations in the comparison. This caused the new link not being found in the set lookup, which leads to erroneously calling notifyListenersAboutAddedElement, when it should've called notifyListenersAboutUpdatedElement instead.

The problem that this PR fixes:

Changing channel config in an .items file, e.g. only the function is being changed from:

String Test { channel="foo" [ profile="transform:REGEX", function="moo" ] }

to:

String Test { channel="foo" [ profile="transform:REGEX", function="cow" ] }

This wouldn't take effect unless a more severe measure was taken, e.g. commenting out the item and re-adding it, or in many cases, users would restart openHAB to reload such changes.

Reported here:
https://community.openhab.org/t/problems-at-reducing-item-changes-with-regex/153400/6

Related:
#3235

This problem may have been started / caused by #1794

Note: I've set the initial capacity for the hashmap to 2, because most items only have 1 link, hopefully it saves a bit of memory by not pre-allocating for 16 slots by default.

Changes in channel config weren't applied because ItemChannelLink.equals() include the link configurations in the comparison. This caused the new link not being found in the set lookup, which leads to erroneously calling notifyListenersAboutAddedElement, when it should've called notifyListenersAboutUpdatedElement instead.

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng jimtng requested a review from a team as a code owner January 31, 2024 02:17
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/problems-at-reducing-item-changes-with-regex/153400/13

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/channel-transformations-not-working-when-using-text-files-for-item-creation/153407/10

Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. (I’m no core maintainer though.)

I think I experienced that issue when attempting to set up a JS transformation for a channel link and I wanted to either open an issue or look into that myself, but you have already solved the problem, great!

Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM. Nice finding.

if (links == null) {
itemChannelLinkMap.put(itemName, links = new HashSet<>());
itemChannelLinkMap.put(itemName, links = new HashMap<>(2));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
itemChannelLinkMap.put(itemName, links = new HashMap<>(2));
itemChannelLinkMap.put(itemName, links = new HashMap<>(2));

Why 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is 16 with .75 load factor. That seems like a waste of memory because most items have just one link.

If I set it to one, as soon as one element is added, it would trigger a rehash and capacity expansion anyway, because it would have exceeded the load factor. So 2 is chosen here.

Tbh this is just a guess based on reading about the description of initial capacity and load factor from hashmap's doc. I haven't done actual memory analysis tests to verify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment in the code for future reference

Comment on lines 135 to 137
for (ItemChannelLink removedItemChannelLink : links.values()) {
notifyListenersAboutRemovedElement(removedItemChannelLink);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (ItemChannelLink removedItemChannelLink : links.values()) {
notifyListenersAboutRemovedElement(removedItemChannelLink);
}
links.values().forEach(this::notifyListenersAboutRemovedElement);

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng jimtng requested a review from J-N-K January 31, 2024 20:58
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Thanks!

@J-N-K J-N-K added the bug An unexpected problem or unintended behavior of the Core label Feb 4, 2024
@J-N-K J-N-K added this to the 4.2 milestone Feb 4, 2024
@J-N-K J-N-K merged commit c929e7d into openhab:main Feb 4, 2024
3 checks passed
@jimtng jimtng deleted the items-file-config-change branch February 4, 2024 12:33
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 the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants