ISPN-14740 Do not persist empty metadata #10798
Conversation
|
Oh, I forgot some persistent tests will fail now due to the smaller size, I will fix those after CI finds them. There shouldn't be any other failures 🤞 |
|
Interestingly this PR seems more likely to reproduce the OOM issue we have in the server test suite that happens with a CompletionStage that has what seems to be an infinitely growing dependency graph. |
|
Replaying CI. |
core/src/main/java/org/infinispan/marshall/persistence/impl/MarshalledEntryFactoryImpl.java
Outdated
Show resolved
Hide resolved
wburns
force-pushed
the
ISPN-14740_persistence_metadata_ignore
branch
from
da2180c
to
9db721b
Compare
|
I have pushed a commit that should reduce how many tasks can pile up from state transfer as it will resume on an executor thread, so it cannot resume again before other tasks are completed and also frees up the non blocking thread for other tasks to be ran first. |
wburns
force-pushed
the
ISPN-14740_persistence_metadata_ignore
branch
from
9db721b
to
fedd938
Compare
|
Two runs without the failure now is looking promising. Running a few more. |
wburns
force-pushed
the
ISPN-14740_persistence_metadata_ignore
branch
from
fedd938
to
57a159c
Compare
core/src/main/java/org/infinispan/marshall/persistence/impl/MarshalledEntryFactoryImpl.java
Show resolved
Hide resolved
|
Looks like it still failed.. so I would need to look into this further. |
|
needs rebase |
wburns
force-pushed
the
ISPN-14740_persistence_metadata_ignore
branch
from
57a159c
to
8e408c7
Compare
|
Rebased and removed the extra testing commit. |
| @@ -205,6 +210,11 @@ public Metadata.Builder builder() { | |||
| return super.builder().lifespan(lifespan).maxIdle(maxIdle); | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean isEmpty() { | |||
| return super.isEmpty() && lifespan < 0 && maxIdle < 0; | |||
There was a problem hiding this comment.
No, 0 has a different meaning or expiration so it has to be <
There was a problem hiding this comment.
what does 0 means? Expire immediately?
| @@ -34,6 +34,11 @@ private MemcachedMetadata(int flags, long lifespan, EntryVersion version) { | |||
| this.flags = flags; | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean isEmpty() { | |||
| return super.isEmpty() && flags != 0; | |||
There was a problem hiding this comment.
typo?
| return super.isEmpty() && flags != 0; | |
| return super.isEmpty() && flags == 0; |
There was a problem hiding this comment.
Yes, good catch. This is from before when I had the code as the inverse (isPresent or w/e I called it). Fixing.
* PersistenceManager won't pass metadata with no expiration or version
wburns
force-pushed
the
ISPN-14740_persistence_metadata_ignore
branch
from
8e408c7
to
4417557
Compare
|
Updated |
|
merged! thanks @wburns ! |
https://issues.redhat.com/browse/ISPN-14740
Reworked this to not change the in memory persistence and instead now we just ignore the metadata when passing to a store.
This saves approximately 40 bytes per entry and avoids serializing the metadata to protostream.
This change is the equivalent to https://github.com/infinispan/infinispan/blob/main/core/src/main/java/org/infinispan/container/impl/InternalEntryFactoryImpl.java#L88 which avoids the allocation in memory.