This repository has been archived by the owner on Nov 14, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[TEX] Part 1b: TrackingKeyValueService: utilities for byte size (1) #6332
[TEX] Part 1b: TrackingKeyValueService: utilities for byte size (1) #6332
Changes from 2 commits
d5cc1e9
be3013b
59bcf56
0c07246
5c686a6
dedfb03
85db6f8
b918778
f551dac
8cec655
cb2208a
ba1732e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
just curious, is the
Long.sum
method call to implicitly widen and avoid casting to long for addition (e.g.return ((long) rowName.length) + columnName.length;
)?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.
yes - it also looked more succinct to my eyes
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.
I assume this is needed as we capture the row name size separately from the columns?
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.
Added only because
getRowName
copies data (& also because the parameter typeT
in row result won't necessarily implementMeasurable
so we can't implement this in-class)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.
These aren't the only strings that are passed around, though. The size of
AbstractKeyValueService.internalTableName()
is probably closer to what you want.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 is only used in the overflow test from what I can see; I'd suggest making it a local to that method.
Also, nit: case is wrong: fields use camelCase (so mockTimestamps)
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.
I think your testing here is very thorough, but I think we can relax it a bit in favor of readability.
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.
It's also "best practice" to try and test a single concept per unit test. So for this, I'd just make a couple like:
void candidateCellSizeHasCorrectSizeForOneTimestamp
,void candidateCellSizeHasCorrectSizeForMultipleTimestamps
,void candidateCellSizeIsEqualRegardlessOfLatestValueEmpty
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.
Out of curiosity, why do we need to fill the array with our default byte? I think all we really care about size, no?
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.
non actionable: I'm not sure if this is a realistic case 😬
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.
seeing as this method just creates a
new byte[size]
, and is only used in one place, consider removing this creation method. although this is a very stylistic nit, so totally feel free to ignore :)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.
nit: also for this and the others, we should name it
createBytes
rather thanspawnBytes
to match codebase style