SLING-7752 - Deserializing and serializing a feature model file suffles the configurations #2
Conversation
…es the configurations
| * @param <K> | ||
| * @param <V> | ||
| */ | ||
| public class OrderedDictionary<K, V> extends Dictionary<K, V> implements Map<K, V> { |
There was a problem hiding this comment.
Do you need the Map implementation as well? In Configuration.java it is only used as a Dictionary implementation.
There was a problem hiding this comment.
@rombert Yes, that is true, in Configuration.java it is only used as Dictionary, but I wanted to maintain backwards compatibility with all the other code that unknowingly used the implementation details of Hashtable. For example I've seen a lot of code doing things like object instance of Map and those conditions would fail.
There was a problem hiding this comment.
My personal preference would be to keep it simple in the beginning and add the extra baggage only when it's needed.
| } | ||
| } | ||
|
|
||
| private Map map = Collections.synchronizedMap(new LinkedHashMap()); |
There was a problem hiding this comment.
There's a slight problem with the thread safety approach here. Collections.syncronizedMap warns that
It is imperative that the user manually synchronize on the returned map when iterating over any of its collection views (...)
We don't expose that for the user. Do we really need a Dictionary implementation or can we replace the properties object with a LinkedHashMap?
There was a problem hiding this comment.
I would be in favour of switching to LinkedHashMap, but the baseline plugin complained that I need to bump the major version since it would change the getter API and I thought I should try like this first. Maybe @bosschaert could give his opinion too on this matter.
There was a problem hiding this comment.
@rombert Looking at the code of SynchronizedMap, Hashtable and LinkedHashMap, it seems to me that the implementation has the same problems with the iterators as the Hashtable too, so the two implementations are equivalent in that regard, just that the Synchronized Map gives you the warning, while the Hashtable lets you figure that out for your self. I think it is the case for almost all collection implementations that Java offers. I could also add the warning to the Javadoc. Do you think we should do more than that or do you have a different recommendation?
There was a problem hiding this comment.
Did not see what we return it using a getter. Yes, that's a problem if changing to a LinkedHashMap, and probably @bosschaert or @cziegeler can tell us if that's a problem.
For the thread safety - I'm not sure we need it. A Hashtable is thread-safe but a Dictionary does not have to be. So IMO we could go with either a LinkedHashMap, while breaking backwards compatibility, or with a non-thread-safe Dictionary implementation backed by a LinkedHashMap.
There was a problem hiding this comment.
We're still at version 0.1.2 which means we didn't do a 1.0 release yet. In OSGi versions starting with 0.x.x indicate 'beta' releases and make no guarantees about backward compatibility, so I think we can apply that reasoning here too.
So I think we can still make things better without being 100% compatible.
The previous use of Dictionary is probably somehow related to the fact that in OSGi configuration properties are stored in Dictionary objects (because this spec predates Java Collections) but I think that where we need Dictionaries we can quite easily convert to them there.
So from me +1 for switching to LinkedHashMap.
|
@rombert just so you know this is just one PR in a series of PRs that will touch: |
…es the configurations
|
@rombert @bosschaert Thank you for the review! I updated the PR according to your review. I changed the API to use |
|
Hi @andreituicu, after seeing the request to also change the Configurator Code in Felix I thought a little more about this... Would it be possible to implement a fix for SLING-7752 without the need to have ordering in the configurator? I put my concerns about changing that one at apache/felix#144 On the BW compatibility plugin and it not considering 0.x versions separately. I'm not sure how to deal with that. @rombert would you have an idea about this? |
If we're sure about what we want the version to be, we can skip the check for a single release, e.g. |
|
Regarding the next version, I would increase the minor version to be 0.2.x. This indicates that it's more than a patch. |
|
@bosschaert Please let me know if there is anything more required here. Thank you! |
|
Hi @andreituicu thanks for this PR. Normally you'd rebase your changes with the latest master rather than merging master out into your branch as that creates a slightly funny looking git history. I'll squash your changes and merge them into master so that history looks normal. Hope that this is ok. |
|
@bosschaert Thank you very much! :) I'll keep the rebasing in mind for future PRs. Some developers prefer merging the master because it doesn't require a |
No description provided.