anton-vinogradov commented on code in PR #13447:
URL: https://github.com/apache/ignite/pull/13447#discussion_r3753940822
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheEntryInfo.java:
##########
@@ -64,58 +68,70 @@ public class GridCacheEntryInfo implements
SelfMarshallingMessage, CacheIdAware
/** Deleted flag. */
private boolean deleted;
- /** {@inheritDoc} */
- @Override public int cacheId() {
- return cacheId;
- }
-
/**
- * @param cacheId Cache ID.
+ * Empty constructor for serialization purposes.
+ * see {@link #expireTimeDelta}.
*/
- public void cacheId(int cacheId) {
+ public GridCacheEntryInfo() {
+ initTime = U.currentTimeMillis();
Review Comment:
Yes, always, and not setting it is a live defect right now.
`expireTime()` starts with `assert initTime > 0`, before the check that
would return 0. For a cache with no expiry policy the constructor skips the `if
(expireTime != 0)` branch, `initTime` stays 0 and the assert throws. It is
thrown inside a future listener and swallowed there, so the near lock future
never completes:
```
(err) Failed to notify listener: GridEmbeddedFuture$1@7be5697c
java.lang.AssertionError
at GridCacheEntryInfo.expireTime(GridCacheEntryInfo.java:131)
at GridNearCacheEntry.initializeFromDht(GridNearCacheEntry.java:156)
at GridNearCacheAdapter.entryEx(GridNearCacheAdapter.java:147)
at GridNearLockFuture$1.apply(GridNearLockFuture.java:1265)
```
`GridCacheNearPrimarySyncSelfTest` on `f95cc504` hangs there for 18 minutes
with a long running transaction until killed.
`GridNearCacheAdapter.entryEx(key, topVer)` calls `initializeFromDht` on every
near entry lookup, so any near cache without an expiry policy hits it.
Setting `initTime` unconditionally costs one volatile read, and the same
test then finishes in 0.25 s with no assertion errors in the log.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheEntryInfo.java:
##########
@@ -64,58 +68,70 @@ public class GridCacheEntryInfo implements
SelfMarshallingMessage, CacheIdAware
/** Deleted flag. */
private boolean deleted;
- /** {@inheritDoc} */
- @Override public int cacheId() {
- return cacheId;
- }
-
/**
- * @param cacheId Cache ID.
+ * Empty constructor for serialization purposes.
+ * see {@link #expireTimeDelta}.
*/
- public void cacheId(int cacheId) {
+ public GridCacheEntryInfo() {
+ initTime = U.currentTimeMillis();
+ }
+
+ /** */
+ public GridCacheEntryInfo(int cacheId, KeyCacheObject key, @Nullable
CacheObject val, GridCacheVersion ver, long expireTime, long ttl) {
+ assert expireTime >= 0;
+
+ if (expireTime != 0) {
+ initTime = U.currentTimeMillis();
+
+ expireTimeDelta = expireTime - initTime;
+
+ // In theory, here we can get the thread paused causing a negative
delta value. Possible negative values
+ // shouldn't be treated as disabled expiration. Correct behavior
is expired timeout.
+ if (expireTimeDelta < 0)
+ expireTimeDelta = 0;
+ }
+
this.cacheId = cacheId;
+ this.key = key;
+ this.val = val;
+ this.ver = ver;
+ this.ttl = ttl;
+ }
+
+ /** {@inheritDoc} */
+ @Override public int cacheId() {
+ return cacheId;
}
/**
* @param key Entry key.
*/
- public void key(KeyCacheObject key) {
+ public void key(@Nullable KeyCacheObject key) {
this.key = key;
}
/**
* @return Entry key.
*/
- public KeyCacheObject key() {
+ @Nullable public KeyCacheObject key() {
return key;
}
/**
* @return Entry value.
*/
- public CacheObject value() {
+ public @Nullable CacheObject value() {
return val;
}
/**
- * @param val Entry value.
- */
- public void value(CacheObject val) {
- this.val = val;
- }
-
- /**
- * @return Expire time.
+ * @return Expire time >= 0. 0 means no expiration is set.
*/
public long expireTime() {
- return expireTime;
- }
+ assert initTime > 0;
+ assert expireTimeDelta >= -1L;
- /**
- * @param expireTime Expiration time.
- */
- public void expireTime(long expireTime) {
- this.expireTime = expireTime;
+ return expireTimeDelta == -1L ? 0L : initTime + expireTimeDelta;
Review Comment:
Two separate ones.
**Overflow.** `initTime + expireTimeDelta` has no guard. The removed
`selfUnmarshal` had it explicitly:
```java
expireTime = remaining < 0 ? 0 : U.currentTimeMillis() + remaining;
// Account for overflow.
if (expireTime < 0)
expireTime = 0;
```
It kept `expireTime()` non negative, which
`GridDhtPartitionDemander.preloadEntry` relies on: `assert row.expireTime() >=
0`. Asserts are off in production, so nothing enforces it there now.
**Only `-1` reads as "no expiration".** The removed `selfUnmarshal` read the
whole negative range that way, `remaining < 0 ? 0 : ...`, where 0 is
`CU.EXPIRE_TIME_ETERNAL`. The new getter compares with exactly `-1L`, so any
other negative value arriving from the wire becomes a time in the past and the
entry gets cleaned, where master kept it as eternal. A correct sender does not
produce such a value, but of the two readings the one that keeps the data is
the safer default, and it also makes `assert expireTimeDelta >= -1L`
unnecessary.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheEntryInfo.java:
##########
@@ -190,30 +192,6 @@ public int marshalledSize(CacheObjectContext ctx) throws
IgniteCheckedException
return SIZE_OVERHEAD + size;
}
- // TODO IGNITE-28920: the rebase still runs inside the message; move it to
the code filling and reading the entry.
- /** {@inheritDoc} */
- @Override public void selfMarshal() {
- if (expireTime == 0)
- expireTime = -1;
- else {
- expireTime -= U.currentTimeMillis();
-
- if (expireTime < 0)
- expireTime = 0;
- }
- }
-
- /** {@inheritDoc} */
- @Override public void selfUnmarshal() {
- long remaining = expireTime;
-
- expireTime = remaining < 0 ? 0 : U.currentTimeMillis() + remaining;
-
- // Account for overflow.
- if (expireTime < 0)
- expireTime = 0;
- }
-
/** {@inheritDoc} */
@Override public String toString() {
Review Comment:
Agreed, nothing is missed for what this constant is used for, but the
constant is muddled and worth a look.
`initTime` is not transferred, and the wire still carries exactly two longs,
`ttl` and `expireTimeDelta`, so for a marshalled size the count is unchanged.
The muddle is that the terms are Java object sizes, `3 * 8 /* reference */`,
while both users of `marshalledSize()` treat the result as bytes on the wire:
`GridDhtPartitionSupplyMessage.addEntry0` cuts supply batches by it against
`rebalanceBatchSize`, and `GridDhtPartitionDemander` feeds it into
`onRebalanceBatchReceived`, the rebalance bytes metric. The real fixed part is
11 bytes measured on a minimal message and around 20 with a realistic cache id
and version, against 76 in the constant, so for small entries batches are
several times smaller than configured and the metric overstates the traffic by
the same factor.
Pre-existing, not this PR, but if the field list is being revisited anyway
it is the moment to decide what the constant estimates.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheMapEntry.java:
##########
@@ -396,23 +396,14 @@ protected GridDhtLocalPartition localPartition() {
try {
if (!obsolete()) {
- info = new GridCacheEntryInfo();
-
- info.key(key);
- info.cacheId(cctx.cacheId());
-
long expireTime = expireTimeExtras();
- boolean expired = expireTime != 0 && expireTime <=
U.currentTimeMillis();
+ CacheObject val0 = expireTime == 0 || expireTime >
U.currentTimeMillis() ? val : null;
Review Comment:
One concrete reason for making it once: right now the decision is split
across two clock reads.
Here `expireTime > U.currentTimeMillis()` decides whether the value goes,
and then the constructor reads the clock again for `expireTime -
U.currentTimeMillis()`. If the two land on different ticks, the entry can be
sent with its value while its remaining time is clamped to 0, that is "expires
right now". The receiver stores a value that is dead on arrival.
Harmless today, but it is exactly the kind of thing that follows from
deciding twice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
