-
Notifications
You must be signed in to change notification settings - Fork 683
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
Further rework of the RestoreRedundancy data objects for serialization #5281
Conversation
This code is to fix another serialization issue with the Restore Redundancy code. Basically, it is very painful from what I have seen to json deserialize a Duration object type. So I have converted the field to a long. |
totalPrimaryTransferTime = | ||
totalPrimaryTransferTime.plusMillis(details.getPrimaryTransferTime()); | ||
totalPrimaryTransferTime + details.getPrimaryTransferTime(); |
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.
This could be replaced with totalPrimaryTransferTime += details.getPrimaryTransferTime();
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.
done
} | ||
|
||
@Test | ||
public void testSerializable() throws JsonProcessingException { |
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.
This test sets multiple fields but does not verify all of them. Would it be possible to confirm that all data in the object is correctly serialized and deserialized, not just the message and status fields?
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.
done. assertThat(value).isEqualToComparingFieldByField(restoreRedundancyResults);
totalPrimaryTransferTime = | ||
totalPrimaryTransferTime.plus(results.getTotalPrimaryTransferTime()); | ||
totalPrimaryTransferTime + results.getTotalPrimaryTransferTime(); |
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.
This can be replaced with totalPrimaryTransferTime += results.getTotalPrimaryTransferTime();
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.
done.
restoreRedundancyResults.addRegionResult(regionRedundancyStatus); | ||
String jsonString = geodeMapper.writeValueAsString(restoreRedundancyResults); | ||
// deserialize the class | ||
RestoreRedundancyResultsImpl 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.
you can use assertJ's isEqualToComparingFieldByField
method to compare all the fields are the same
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.
done
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.
did you forget to push your commit?
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.
Nope it is failing... It doesn't like the objects in the satisfiedregion lists.
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 hash maps.
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.
assertThat(value).usingRecursiveComparison().isEqualTo(restoreRedundancyResults);
worked
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.
otherwise LGTM
…erialization (apache#5281) * Converting a Duration to a Long because of Serialization * Changes for per requests
looking for errors.