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

Move read-only config to internal package #15569

Merged
merged 5 commits into from Sep 20, 2019

Conversation

@mmedenjak
Copy link
Contributor

mmedenjak commented Sep 18, 2019

Moves all read-only config classes to internal package, removes the
deprecation annotations as these classes will stay in 4.0 and marks the
getAsReadOnly methods as PrivateApi.

@mdogan
mdogan approved these changes Sep 19, 2019
Copy link
Member

mdogan left a comment

Should we also mention in javadoc of these methods that they are private api and for internal use only, in addition to @PrivateApi annotation?

Copy link
Member

devozerov left a comment

Looks good to me with respect to the original description of the ticket. A minor comment - it looks like we can get rid of private getAsReadOnly method in configs if we just inverse the control: instead of someConfig.getAsReadOnly() do new SomeConfigReadOnly(someConfig). This may produce a bit more litter, but the usage of "read-only" functionality doesn't appear to be on a hot path.

@mmedenjak

This comment has been minimized.

Copy link
Contributor Author

mmedenjak commented Sep 19, 2019

@devozerov yes, I thought of that. But I was reluctant as I was unsure about the GC pressure and I vaguely remembered some regression where the test was doing create-use-destroy for many instances with unique names, eventually leading to OOME because new config instances were created and added to a collection. But I may be just mixing things so I'll take a deeper look before merging. In the light of this, I might remove the method altogether. WDYT @mdogan ?

@mdogan

This comment has been minimized.

Copy link
Member

mdogan commented Sep 19, 2019

I am more than ok with the new approach, it's better for the public API.

@mmedenjak mmedenjak requested a review from hazelcast/clients as a code owner Sep 19, 2019
@sancar
sancar approved these changes Sep 19, 2019
@mmedenjak mmedenjak force-pushed the mmedenjak:4.0-read-only-config branch from 50455c2 to bf9a04b Sep 19, 2019
Matko Medenjak added 5 commits Sep 18, 2019
Moves all read-only config classes to internal package, removes the
deprecation annotations as these classes will stay in 4.0 and marks the
getAsReadOnly methods as PrivateApi.
Matko Medenjak
@mmedenjak mmedenjak force-pushed the mmedenjak:4.0-read-only-config branch from bf9a04b to c3be7cd Sep 20, 2019
@mmedenjak

This comment has been minimized.

Copy link
Contributor Author

mmedenjak commented Sep 20, 2019

@devozerov @mdogan addressed the review comments, now there is no getAsReadOnly or similar method on any config. I moved some *ReadOnly inner classes to the internal package and fixed visibility modifiers. I also checked the regression issue with OOME I was talking about and checked the usage of Config#findMapConfig as a sample and saw it is not on any hot path. As such, I'm merging the PR. We should keep a close eye in the stabilisation period on test results to see we don't have any unforeseen performance regression or memory leak.
Thank you all for your reviews!

@mmedenjak mmedenjak merged commit 58c2cdb into hazelcast:master Sep 20, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@mmedenjak mmedenjak deleted the mmedenjak:4.0-read-only-config branch Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.