Datastore: expose serializedSize for Entity. - #6842
Conversation
Sorry, something went wrong.
| return properties; | ||
| } | ||
|
|
||
| /** Returns the entity size. */ |
There was a problem hiding this comment.
"Returns the number of bytes in the serialized protobuf."
Is this definitely a fixed and reproducible value for a given protobuf?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes,Once build it would be fixed number.
Sorry, something went wrong.
| } | ||
|
|
||
| @Test | ||
| public void testEntitySize() throws Exception { |
There was a problem hiding this comment.
Does this really throw exception?
Sorry, something went wrong.
There was a problem hiding this comment.
No,This method does not throw exception.
Sorry, something went wrong.
| BaseEntity(Builder<K, ?> builder) { | ||
| this.key = builder.key; | ||
| this.properties = ImmutableSortedMap.copyOf(builder.properties); | ||
| this.serializedSize = this.toPb().getSerializedSize(); |
There was a problem hiding this comment.
what's the cost of calculating this for a large entity? Most of the time we don't need this value. Perhaps this could be done in demand and not stored or only lazily stored in the field?
Sorry, something went wrong.
There was a problem hiding this comment.
The cost could be varied it depends on user entered properties. Does this look fine?
/** Returns the number of bytes in the serialized protobuf. */
public int getSerializedSize() {
if (serializedSize == 0) {
serializedSize = this.toPb().getSerializedSize();
}
return serializedSize;
}Here we are calculating entity size when demand.
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #6842 +/- ##
=============================================
- Coverage 45.72% 45.28% -0.45%
+ Complexity 26528 6251 -20277
=============================================
Files 2615 453 -2162
Lines 282766 56282 -226484
Branches 32745 7229 -25516
=============================================
- Hits 129299 25486 -103813
+ Misses 143350 29140 -114210
+ Partials 10117 1656 -8461
Continue to review full report at Codecov.
|
Sorry, something went wrong.
BenWhitehead
left a comment
There was a problem hiding this comment.
I've spent some time thinking about this and I feel like this is more of a utility method than a property of the entity. Entities are immutable, but in order to not incur a penalty of adding making size available serializedSize would need to be non-final which opens up other issues.
Instead of this being a field with a getter, can you add a new static method to com.google.cloud.datastore.Entity:
public static int calculateSerializedSize(BaseEntity<? extends IncompleteKey> e) {
return e.toPb().getSerializedSize();
}This way we don't impose a penalty to those users that don't need this operation.
Sorry, something went wrong.
| return new Builder().fill(entityPb).build(); | ||
| } | ||
|
|
||
| public static int calculateSerializedSize(BaseEntity<? extends IncompleteKey> e) { |
There was a problem hiding this comment.
Please add a javadoc comment to explain the usage of this method (and to help people find it when they google for this type of thing).
Sorry, something went wrong.
| /** | ||
| * Returns the serialized protobuf size of provided entity object. | ||
| * | ||
| * @param entity entity object to calculate serialize size. |
There was a problem hiding this comment.
"object whose size is measured"
Sorry, something went wrong.
| checkNotNull(e); | ||
| return e.toPb().getSerializedSize(); | ||
| /** | ||
| * Returns the serialized protobuf size of provided entity object. |
There was a problem hiding this comment.
"Returns the size in bytes of the protobuf form of the provided entity."
Sorry, something went wrong.
* determine entity size using api * organize individual import * updated code * update code for testcase * fix review changes * fix review changes * fix review changes
Fixes #6389