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.

Sign up for GitHub

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

Jump to bottom

feature-25363 : Deprecating size based cache at heap tier #3038

Merged
merged 1 commit into from
Aug 8, 2022
Merged

feature-25363 : Deprecating size based cache at heap tier #3038

merged 1 commit into from
Aug 8, 2022

Conversation

nnares
Copy link
Contributor

@nnares nnares commented Jun 23, 2022

No description provided.

@jhouserizer
Copy link
Member

hi! shouldn't there also be some deprecation on Resource Pools?

chrisdennis
chrisdennis requested changes Jun 29, 2022
View reviewed changes
ehcache-impl/src/main/java/org/ehcache/config/builders/CacheConfigurationBuilder.java Outdated Show resolved Hide resolved
ehcache-core/src/main/java/org/ehcache/core/spi/store/heap/SizeOfEngineProvider.java Outdated Show resolved Hide resolved
ehcache-impl/src/main/java/org/ehcache/config/builders/CacheManagerBuilder.java Outdated Show resolved Hide resolved
ehcache-impl/src/main/java/org/ehcache/impl/internal/sizeof/DefaultSizeOfEngineProvider.java Outdated Show resolved Hide resolved
ehcache-impl/src/test/java/org/ehcache/config/builders/CacheConfigurationBuilderTest.java Outdated Show resolved Hide resolved
ehcache-core/src/main/java/org/ehcache/core/spi/store/heap/SizeOfEngineProvider.java Outdated Show resolved Hide resolved
@linghengqian
Copy link

Related to #3008 ? I'm a little curious about the feature-25363 post.

@nnares
Copy link
Contributor Author

nnares commented Jul 4, 2022

Related to #3008 ? I'm a little curious about the feature-25363 post.

No, This is related to the itrac feature - https://itrac.eur.ad.sag/browse/WF-25363

@chrisdennis
Copy link
Member

Related to #3008 ? I'm a little curious about the feature-25363 post.

No, This is related to the itrac feature - https://itrac.eur.ad.sag/browse/WF-25363

Of course that's an internal only JIRA instance... we're softly deprecating the byte based sizing of heap resources in 3.x. The size-of implementation is fast becoming unsupportable/maintainable so we'd like users to migrate away from it if possible. It's likely that in 4.x (when we'll be dropping Java 8 support) that we'll move byte based sizing out of the main distribution. It will probably be recoverable by adding additional jars to the classpath but the anticipation is that it will be a "second class citizen".

chrisdennis
chrisdennis requested changes Jul 13, 2022
View reviewed changes
Copy link
Member

@chrisdennis chrisdennis left a comment

Choose a reason for hiding this comment

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

I like the overall approach here (I think) but why didn't you deprecate the SizeOfEngine itself, or the associated exception types?

We should also think about putting some logging in to report use of the deprecated APIs.

@linghengqian linghengqian mentioned this pull request Jul 13, 2022
Closed
@nnares
Copy link
Contributor Author

nnares commented Jul 21, 2022

I like the overall approach here (I think) but why didn't you deprecate the SizeOfEngine itself, or the associated exception types?

We should also think about putting some logging in to report use of the deprecated APIs.

Have marked both SizeOfEngine & LimitExceededException as @Deprecated

@chrisdennis chrisdennis self-requested a review July 21, 2022 19:25
chrisdennis
chrisdennis requested changes Jul 21, 2022
View reviewed changes
Copy link
Member

@chrisdennis chrisdennis left a comment

Choose a reason for hiding this comment

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

See my PR against your branch for comments/fixes to this. We should also think about some logging to warn of the deprecated usage, and we also need to deprecate (in some appropriate manner) the relevant XML schema components.

@chrisdennis chrisdennis self-requested a review July 27, 2022 19:52
chrisdennis
chrisdennis requested changes Jul 27, 2022
View reviewed changes
Copy link
Member

@chrisdennis chrisdennis left a comment

Choose a reason for hiding this comment

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

Logging needs fixing to catch XML configuration cases. Also don't forget to find some way to annotate the relevant XML schema elements as deprecated.

ehcache-impl/src/main/java/org/ehcache/config/builders/ResourcePoolsBuilder.java Outdated Show resolved Hide resolved
chrisdennis
chrisdennis requested changes Aug 4, 2022
View reviewed changes
ehcache-impl/src/main/java/org/ehcache/impl/config/SizedResourcePoolImpl.java Outdated
@@ -55,7 +57,7 @@ public SizedResourcePoolImpl(ResourceType<P> type, long size, ResourceUnit unit,
throw new IllegalStateException("Non-persistable resource cannot be configured persistent");
}
if (type == org.ehcache.config.ResourceType.Core.HEAP && unit instanceof MemoryUnit){
LoggerFactory.getLogger(SizedResourcePoolImpl.class).info("Byte based heap resources are deprecated now and will be removed completely in future version.");
LOGGER.info("Size based heap resources are deprecated and will be removed in a future version.");
Copy link
Member

Choose a reason for hiding this comment

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

No need to remove the Byte based, the logging is now either vague or wrong. Entry based heap resources are still sized, and still supported. This can go back to:

Byte based heap resources are deprecated and will be removed in a future version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accommodated - Placed same statement

ehcache-xml/src/common/schema/ehcache-core.xsd Outdated
@@ -289,7 +290,7 @@
<xs:annotation>
<xs:documentation xml:lang="en">
Shortcut for configuring a heap Cache.
Byte based heap resources are deprecated now and will be removed completely in future version.
Deprecated (for memory units)
Copy link
Member

Choose a reason for hiding this comment

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

Well now this is even more ambiguous. Something like this was what I meant:


DEPRECATED: The use of memory units (such as 'B', 'kB' or 'MB') for heap resources is deprecated and will be removed in a future version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accommodated - Placed same statement

ehcache-xml/src/common/schema/ehcache-core.xsd Outdated
@@ -305,6 +306,7 @@
<xs:annotation>
<xs:documentation xml:lang="en">
The element defines the sizing limits for the Cache's SizeOfEngine.
Deprecated (for memory units)
Copy link
Member

Choose a reason for hiding this comment

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

I didn't mean to include these statements directly... I was only pointing out which things needed documentation, not precisely what language to use.

For this element something like this should work:


DEPRECATED: Memory units for heap resources, which are the primary use-case for size-of-engines, are deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accommodated - Placed same statement

ehcache-xml/src/common/schema/ehcache-core.xsd
@@ -538,6 +540,9 @@
</xs:appinfo>
<xs:appinfo>
<annox:annotate>@java.lang.SuppressWarnings({"unchecked", "serial"})</annox:annotate>
<xs:documentation>
Copy link
Member

Choose a reason for hiding this comment

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

For this element we should be able to lift the comment from the other heap element.


DEPRECATED: The use of memory units (such as 'B', 'kB' or 'MB') for heap resources is deprecated and will be removed in a future version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accommodated - Placed same statement

ehcache-xml/src/common/schema/ehcache-core.xsd Outdated
@@ -90,6 +90,7 @@
<xs:annotation>
<xs:documentation xml:lang="en">
The element defines the sizing limits for the default SizeOfEngine.
Depricated (for memory units)
Copy link
Member

Choose a reason for hiding this comment

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

For this element something like this should work:


DEPRECATED: Memory units for heap resources, which are the primary use-case for size-of-engines, are deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accommodated - Placed same statement

@chrisdennis chrisdennis self-requested a review August 5, 2022 13:21
@nnares nnares force-pushed the feature-25363 branch 2 times, most recently from 0881efd to f0e769f Compare August 5, 2022 14:41
@chrisdennis chrisdennis merged commit c09439b into ehcache:master Aug 8, 2022
@linghengqian linghengqian mentioned this pull request Mar 30, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisdennis chrisdennis chrisdennis approved these changes

@AbfrmBlr AbfrmBlr Awaiting requested review from AbfrmBlr

@jhouserizer jhouserizer Awaiting requested review from jhouserizer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nnares @jhouserizer @linghengqian @chrisdennis @mrpiggi