[improve] Allow the content be empty when deserialize in MetadataCache #23131
Conversation
--- ### Motivation When using admin to create topic, it will set the content to byte[0] then put it into the value. Sometimes we need to update the value then we will received the deserailization error. So we need to check the value type to make it as optional empty when the value is null.
|
@zymap Please add the following content to your PR description and select a checkbox: |
zymap
changed the title
github-actions
bot
added
doc-not-needed
zymap
requested review from
merlimat,
lhotari,
hangc0276,
RobertIndie and
horizonzy
| } catch (IOException e) { | ||
| return FutureUtils.exception(e); | ||
| } | ||
| currentValue = Optional.of(clone); |
There was a problem hiding this comment.
instead of making the change above, Optional.of could be changed to Optional.ofNullable to achieve the same outcome.
| if (res.getValue().length == 0) { | ||
| return FutureUtils.value(Optional.of(new CacheGetResult<>(null, res.getStat()))); | ||
| } |
There was a problem hiding this comment.
There would be less code duplication if this change would be handled in a way where obj is set to null when res.getValue().length == 0
There was a problem hiding this comment.
for example T obj = res.getValue().length > 0 ? serde.deserialize(path, res.getValue(), res.getStat()) : null;
There was a problem hiding this comment.
BookieServiceInfo handled the case of res.getValue.length==0 in deserialize
That's why the test TestAutoRecoveryAlongWithBookieServers#testAutoRecoveryAlongWithBookieServers failed.
Maybe we need to deserialize first and add the res.getValue.length==0 check in the exception section?
There was a problem hiding this comment.
@hangc0276 Thank you! I realized I was wrong to do this fix here because the byte[0] may be a magic value for the SerDe implementation. It should be handled in the SerDe classes.
| if (res.getValue().length == 0) { | ||
| return FutureUtils.value(Optional.of(new CacheGetResult<>(null, res.getStat()))); | ||
| } |
There was a problem hiding this comment.
BookieServiceInfo handled the case of res.getValue.length==0 in deserialize
That's why the test TestAutoRecoveryAlongWithBookieServers#testAutoRecoveryAlongWithBookieServers failed.
Maybe we need to deserialize first and add the res.getValue.length==0 check in the exception section?
lhotari
added
release/3.3.3
release/3.0.8
and removed
release/3.3.2
release/3.0.7
labels
Motivation
When using admin to create topic, it will set the content to byte[0] then put it into the value. Sometimes we need to update the value then we will received the deserailization error.
So we need to check the value type to make it as optional empty when the value is null.
Fixes #xyz
Main Issue: #xyz
PIP: #xyz
Motivation
Modifications
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: