[TEX] Part 1b: TrackingKeyValueService: utilities for byte size (1) #6332

ergo14 · 2022-10-30T13:02:41Z

General

Before this PR:

After this PR:
sizeInBytes methods in existing classes of interest.
==COMMIT_MSG==
==COMMIT_MSG==

Concerns / possible downsides (what feedback would you like?):
Are the tests enough? Pointers on how to test the rest

Is documentation needed?:
No

Compatibility

Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?:
No
Does this PR change the persisted format of any data - if so, do we have forward and backward compatibility?:
No
The code in this PR may be part of a blue-green deploy. Can upgrades from previous versions safely coexist? (Consider restarts of blue or green nodes.):
Yes
Does this PR rely on statements being true about other products at a deployment - if so, do we have correct product dependencies on these products (or other ways of verifying that these statements are true)?:
No
Does this PR need a schema migration?
No

Testing and Correctness

What, if any, assumptions are made about the current state of the world? If they change over time, how will we find out?:
N/A
What was existing testing like? What have you done to improve it?:
Added tests for added iterators
If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.:
N/A
If this PR involves acquiring locks or other shared resources, how do we ensure that these are always released?:
N/A

Execution

How would I tell this PR works in production? (Metrics, logs, etc.):
N/A
Has the safety of all log arguments been decided correctly?:
N/A
Will this change significantly affect our spending on metrics or logs?:
N/A
How would I tell that this PR does not work in production? (monitors, etc.):
N/A
If this PR does not work as expected, how do I fix that state? Would rollback be straightforward?:
recall and rollback
If the above plan is more complex than “recall and rollback”, please tag the support PoC here (if it is the end of the week, tag both the current and next PoC):
N/A

Scale

Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.:
No
Would this PR be expected to perform a large number of database calls, and/or expensive database calls (e.g., row range scans, concurrent CAS)?:
No
Would this PR ever, with time and scale, become the wrong thing to do - and if so, how would we know that we need to do something differently?:
No

Development Process

Where should we start reviewing?:
Cell.java
If this PR is in excess of 500 lines excluding versions lock-files, why does it not make sense to split it?:
N/A

changelog-app · 2022-10-30T13:02:44Z

Generate changelog in `changelog/@unreleased`

Type

Description

Transactional Expectations Part 1b1: TrackingKeyValueService: utilities for byte size (1)

Check the box to generate changelog(s)

Generate changelog entry

atlasdb-api/src/main/java/com/palantir/atlasdb/keyvalue/api/Value.java

schlosna · 2022-11-01T19:18:39Z

atlasdb-api/src/main/java/com/palantir/atlasdb/keyvalue/api/Cell.java

@@ -116,6 +116,10 @@ public byte[] getColumnName() {
        return columnName;
    }

+    public long sizeInBytes() {
+        return Long.sum(rowName.length, columnName.length);


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;)?

yes - it also looked more succinct to my eyes

Co-authored-by: David Schlosnagle <[email protected]>

Sam-Kramer

Just some general nits/styles stuff

atlasdb-api/src/main/java/com/palantir/atlasdb/keyvalue/api/Cell.java

atlasdb-api/src/main/java/com/palantir/atlasdb/keyvalue/api/TableReference.java

Sam-Kramer · 2022-11-02T09:51:58Z

atlasdb-api/src/main/java/com/palantir/atlasdb/keyvalue/api/RowResult.java

@@ -68,6 +68,10 @@ public byte[] getRowName() {
        return row.clone();
    }

+    public long getRowNameSize() {


I assume this is needed as we capture the row name size separately from the columns?

Added only because getRowName copies data (& also because the parameter type T in row result won't necessarily implement Measurable so we can't implement this in-class)

Sam-Kramer · 2022-11-02T10:02:31Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/CandidateCellForSweepingTest.java

+        return Collections.nCopies(size, TIMESTAMP);
+    }
+
+    private static byte[] spawnBytes(int size) {


Out of curiosity, why do we need to fill the array with our default byte? I think all we really care about size, no?

Sam-Kramer · 2022-11-02T10:04:40Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/CandidateCellForSweepingTest.java

+    private List<Long> MOCK_TIMESTAMPS;
+
+    @Test
+    public void candidateCellSizeWithLargerTimestampCollectionIsBigger() {


I think your testing here is very thorough, but I think we can relax it a bit in favor of readability.

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

Sam-Kramer · 2022-11-02T13:44:49Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/CellTest.java

-                .collect(Collectors.toSet());
+    private static Cell createCellWithByteSize(int size) {
+        Preconditions.checkArgument(size > 1, "Size should be at least 2");
+        return Cell.create(spawnBytes(size / 2), spawnBytes(size - (size / 2)));


Maybe I'm being a bit thick, but shouldn't size / 2 == size - (size / 2) the vast majority of the time?

They are equal when n is even

why do you want them to not be equal when n is odd?

I think @ergo14 wants the cell to have exactly the right size (e.g., otherwise createCellWithByteSize(7) returns a cell that has row and column of size 3, i.e. an overall size of 6) - it'll be one byte smaller if we just divide when size is odd

Sam-Kramer · 2022-11-02T13:45:46Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/CandidateCellForSweepingTest.java

-        Arrays.fill(bytes, BYTE);
-        return bytes;
+    private static byte[] spawnBytes() {
+        return new byte[CELL_NAME_SIZE];


tbh I'd just make this a static variable at this point

Sam-Kramer · 2022-11-02T13:50:09Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/ValueTest.java

+    }
+
+    private static Value createValue(int contentsSize) {
+        return Value.create(spawnBytes(contentsSize), Value.INVALID_VALUE_TIMESTAMP);
    }

    private static byte[] spawnBytes(int size) {


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 :)

nit: also for this and the others, we should name it createBytes rather than spawnBytes to match codebase style

jeremyk-91

One key question around the question of how we compute the size. The rest is broadly fine.

jeremyk-91 · 2022-11-02T17:17:42Z

atlasdb-api/src/main/java/com/palantir/atlasdb/keyvalue/api/TableReference.java

@@ -146,6 +148,15 @@ public String toString() {
        return getQualifiedName();
    }

+    @Override
+    public long sizeInBytes() {
+        return stringSizeInBytes(tableName) + stringSizeInBytes(namespace.getName());


These aren't the only strings that are passed around, though. The size of AbstractKeyValueService.internalTableName() is probably closer to what you want.

jeremyk-91 · 2022-11-02T17:18:33Z

atlasdb-api/src/main/java/com/palantir/atlasdb/util/Measurable.java

+package com.palantir.atlasdb.util;
+
+public interface Measurable {
+    long sizeInBytes();


Maybe trivial, but I would add javadocs.

jeremyk-91 · 2022-11-02T17:19:36Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/CandidateCellForSweepingTest.java

+    @Mock
+    private List<Long> MOCK_TIMESTAMPS;


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)

jeremyk-91 · 2022-11-02T17:21:20Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/CellTest.java

+    }
+
+    private static Cell createCellWithByteSize(int size) {
+        Preconditions.checkArgument(size > 1, "Size should be at least 2");


nit: since the assertion is at least 2, no need to rely on the discrete domain - just write size >= 2?

jeremyk-91 · 2022-11-02T17:22:10Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/TableReferenceTest.java

+
+    @Test
+    public void sizeInBytesForTableReferenceWithEmptyNamespaceIsSizeOfAsciiTableName() {
+        assertThat(TableReference.createWithEmptyNamespace("").sizeInBytes()).isEqualTo(0);


non actionable: I'm not sure if this is a realistic case 😬

jeremyk-91

👍

svc-autorelease · 2022-11-08T23:15:17Z

Released 0.757.0

copy pasta

d5cc1e9

ergo14 mentioned this pull request Oct 30, 2022

[TEX] Part 1c: TrackingKeyValueService: utilities for byte size (2) #6334

Merged

schlosna reviewed Nov 1, 2022

View reviewed changes

fix comment

be3013b

Co-authored-by: David Schlosnagle <[email protected]>

Sam-Kramer reviewed Nov 2, 2022

View reviewed changes

ergo14 added 2 commits November 2, 2022 13:36

sk review flups

59bcf56

Merge branch 'tex-pr-1b1' of github.com:palantir/atlasdb into tex-pr-1b1

0c07246

Sam-Kramer reviewed Nov 2, 2022

View reviewed changes

test refactoring

5c686a6

Sam-Kramer approved these changes Nov 2, 2022

View reviewed changes

jeremyk-91 reviewed Nov 2, 2022

View reviewed changes

ergo14 added 3 commits November 2, 2022 18:55

some 2nd pass flups

dedfb03

make test classes final

85db6f8

style

b918778

ergo14 changed the title ~~[TEX] Part 1b1: TrackingKeyValueService: utilities for byte size (1)~~ [TEX] Part 1b: TrackingKeyValueService: utilities for byte size (1) Nov 4, 2022

review flups

f551dac

jeremyk-91 approved these changes Nov 8, 2022

View reviewed changes

Add generated changelog entries

8cec655

ergo14 added autorelease merge when ready labels Nov 8, 2022

ergo14 added 2 commits November 8, 2022 14:59

Merge branch 'develop' into tex-pr-1b1

cb2208a

Merge branch 'develop' into tex-pr-1b1

ba1732e

bulldozer-bot bot merged commit 6eaa70d into develop Nov 8, 2022

ergo14 mentioned this pull request Mar 21, 2023

[TEX] Wrap KVS service reads and track bytes read (no metrics) #6475

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEX] Part 1b: TrackingKeyValueService: utilities for byte size (1) #6332

[TEX] Part 1b: TrackingKeyValueService: utilities for byte size (1) #6332

ergo14 commented Oct 30, 2022 •

edited

Loading

changelog-app bot commented Oct 30, 2022 •

edited by ergo14

Loading

schlosna Nov 1, 2022

ergo14 Nov 1, 2022

Sam-Kramer left a comment

Sam-Kramer Nov 2, 2022

ergo14 Nov 2, 2022

Sam-Kramer Nov 2, 2022

Sam-Kramer Nov 2, 2022

Sam-Kramer Nov 2, 2022

Sam-Kramer Nov 2, 2022

ergo14 Nov 2, 2022

Sam-Kramer Nov 2, 2022

jeremyk-91 Nov 2, 2022 •

edited

Loading

Sam-Kramer Nov 2, 2022

Sam-Kramer Nov 2, 2022 •

edited

Loading

Sam-Kramer Nov 2, 2022

jeremyk-91 left a comment

jeremyk-91 Nov 2, 2022

jeremyk-91 Nov 2, 2022

jeremyk-91 Nov 2, 2022

jeremyk-91 Nov 2, 2022

jeremyk-91 Nov 2, 2022 •

edited

Loading

jeremyk-91 left a comment

svc-autorelease commented Nov 8, 2022

[TEX] Part 1b: TrackingKeyValueService: utilities for byte size (1) #6332

[TEX] Part 1b: TrackingKeyValueService: utilities for byte size (1) #6332

Conversation

ergo14 commented Oct 30, 2022 • edited Loading

General

Compatibility

Testing and Correctness

Execution

Scale

Development Process

changelog-app bot commented Oct 30, 2022 • edited by ergo14 Loading

Generate changelog in changelog/@unreleased

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sam-Kramer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk-91 Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sam-Kramer Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk-91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk-91 Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

jeremyk-91 left a comment

Choose a reason for hiding this comment

svc-autorelease commented Nov 8, 2022

ergo14 commented Oct 30, 2022 •

edited

Loading

changelog-app bot commented Oct 30, 2022 •

edited by ergo14

Loading

Generate changelog in `changelog/@unreleased`

jeremyk-91 Nov 2, 2022 •

edited

Loading

Sam-Kramer Nov 2, 2022 •

edited

Loading

jeremyk-91 Nov 2, 2022 •

edited

Loading