-
Notifications
You must be signed in to change notification settings - Fork 3k
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 previous value of EmptyCache map representation #14702
Fix previous value of EmptyCache map representation #14702
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Would it be possible to have a test for this?
for empty and non-empty caches?
2a79241
to
cf23167
Compare
Added tests. PTAL |
@@ -116,7 +116,7 @@ public ConcurrentMap<K, V> asMap() | |||
public V putIfAbsent(K key, V value) | |||
{ | |||
// Cache, even if configured to evict everything immediately, should allow writes. | |||
return value; | |||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the above comment looks weird now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed myself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you forced pushed.
Would you consider
// putIfAbsent returns the previous value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that 🤦
Notice that the put method has the old comment still, I find old comment still useful. It is more about empty cache, not the put method. I will leave both ones.
cf23167
to
8ad23f4
Compare
lib/trino-collect/src/main/java/io/trino/collect/cache/EvictableCacheBuilder.java
Show resolved
Hide resolved
lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java
Outdated
Show resolved
Hide resolved
lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Test(dataProvider = "disabledCacheImplementations") | ||
public void testPutOnNonEmptyCache(DisabledCacheImplementation disabledCacheImplementation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache constructed in this test looks to be empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to stress that it is not EmptyCache implementation
lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java
Outdated
Show resolved
Hide resolved
lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java
Outdated
Show resolved
Hide resolved
lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java
Outdated
Show resolved
Hide resolved
lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java
Outdated
Show resolved
Hide resolved
8ad23f4
to
b7ac8d0
Compare
lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java
Outdated
Show resolved
Hide resolved
b7ac8d0
to
d8a631c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Move DataProviders to trino-testing-services" LGTM
<exclusion> | ||
<!-- conflicts with test-scoped dependency declared below --> | ||
<groupId>org.openjdk.jmh</groupId> | ||
<artifactId>jmh-core</artifactId> | ||
</exclusion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me do it as follow up.
@@ -116,7 +116,7 @@ public ConcurrentMap<K, V> asMap() | |||
public V putIfAbsent(K key, V value) | |||
{ | |||
// Cache, even if configured to evict everything immediately, should allow writes. | |||
return value; | |||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you forced pushed.
Would you consider
// putIfAbsent returns the previous value
lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Test | ||
public void testPutOnNonEmptyCache() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache under test is empty.
Add a comment capturing #14702 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to rename the test
putIfAbsent returns previous value and since EmptyCache is not storing anything there is no previous value, so null should be returned.
d8a631c
to
5d3202f
Compare
Do we need a release note for this? |
No. It is not changing anything from point of the view of user. |
Fix previous value of EmptyCache map representation
putIfAbsent returns previous value and since EmptyCache is not storing
anything there is no previous value, so null should be returned.