[TEX] Part 3: TrackingKeyValueService: tracking iterator utils #6336

ergo14 · 2022-10-30T22:23:49Z

General

Before this PR:

After this PR:
Adding some wrapper iterator classes for data tracking.

==COMMIT_MSG==
==COMMIT_MSG==

Priority:

Concerns / possible downsides (what feedback would you like?):

Is documentation needed?:

Compatibility

Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?:

Does this PR change the persisted format of any data - if so, do we have forward and backward compatibility?:

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

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

Does this PR need a schema migration?

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

What was existing testing like? What have you done to improve it?:

If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.:

If this PR involves acquiring locks or other shared resources, how do we ensure that these are always released?:

Execution

How would I tell this PR works in production? (Metrics, logs, etc.):

Has the safety of all log arguments been decided correctly?:

Will this change significantly affect our spending on metrics or logs?:

How would I tell that this PR does not work in production? (monitors, etc.):

If this PR does not work as expected, how do I fix that state? Would rollback be straightforward?:

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

Scale

Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.:

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

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

Development Process

Where should we start reviewing?:
Tracking

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-30T22:23:52Z

Generate changelog in `changelog/@unreleased`

Type

Description

Transactional Expectations Part 1d1: TrackingKeyValueService: tracking iterator utils

Check the box to generate changelog(s)

Generate changelog entry

Sam-Kramer

Just a couple of clarifications otherwise looks good!

Sam-Kramer · 2022-11-02T13:54:53Z

...hared/src/main/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIterator.java

+import java.util.function.Function;
+
+public class TrackingIterator<T, I extends Iterator<T>> extends ForwardingIterator<T> {
+    I delegate;


make these private variables :)

Sam-Kramer · 2022-11-02T15:49:25Z

.../java/com/palantir/atlasdb/transaction/impl/expectations/TrackingRowColumnRangeIterator.java

+import java.util.function.Consumer;
+import java.util.function.Function;
+
+public class TrackingRowColumnRangeIterator extends TrackingIterator<Map.Entry<Cell, Value>, RowColumnRangeIterator>


Do we think TrackingRowColumnRangeIterator will be extended or not? If not, it may make sense to make this class final.

Sam-Kramer · 2022-11-02T15:49:57Z

...st/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingClosableIteratorTest.java

+        verify(iterator, times(1)).close();
+    }
+
+    private static ClosableIterator<String> spawnClosableIterator() {


nit: createClosableIterator()

Sam-Kramer · 2022-11-02T15:53:40Z

...st/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingClosableIteratorTest.java

+    @Test
+    public void trackingClosableStringIteratorDelegatesClose() {
+        ClosableIterator<String> iterator = spy(spawnClosableIterator());
+        TrackingClosableIterator<String> trackingIterator = new TrackingClosableIterator<>(iterator, noOp(), MEASURER);


nit (non-blocking): I think we should add one test here which actually validates that our tracker and measurer were passed to TrackingClosableIterator correctly. I'd just do this by creating a one-item iterator, iterate over it, and then check and see that tracker/measurer are called with the right things.

Sam-Kramer · 2022-11-02T16:03:08Z

...d/src/test/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIteratorTest.java

+
+    @Test
+    public void trackingStringIteratorTracksData() {
+        Iterator<String> iterator = spawnIterator();


this is unused!

Sam-Kramer · 2022-11-02T16:05:32Z

...d/src/test/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIteratorTest.java

+    public void trackingStringIteratorTracksData() {
+        Iterator<String> iterator = spawnIterator();
+
+        TrackingIterator<String, Iterator<String>> trackingIterator = new TrackingIterator<>(


can do this instead:

Iterator<String> iterator = spawnIterator(); TrackingIterator<String, Iterator<String>> trackingIterator = new TrackingIterator<>( spawnIterator(), bytes -> assertThat(bytes).isEqualTo(MEASURER.apply(iterator.next())), MEASURER); trackingIterator.forEachRemaining(noOp());

Also be good to add a test that ensures that the tracker has been called (plus has the expected total amount)

Sam-Kramer · 2022-11-02T16:12:43Z

...d/src/test/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIteratorTest.java

+        return STRINGS.stream().iterator();
+    }
+
+    private static <T> Consumer<T> noOp() {


Can also make this static if you want

I suppose you mean making this a static field instead of a static method - the reason I am having it this way is so it can be used as a consumer of any generic type. Is there any other way of doing that?

Sam-Kramer · 2022-11-02T16:13:08Z

...d/src/test/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIteratorTest.java

+
+    @Test
+    public void trackingStringIteratorForwardsData() {
+        Iterator<String> iterator = spawnIterator();


You could also test this as:

Iterator<String> iterator = spawnIterator(); TrackingIterator<String, Iterator<String>> trackingIterator = new TrackingIterator<>(spawnIterator(), noOp(), MEASURER); assertThatIterator(trackingIterator).toIterable().containsExactlyElementsOf(ImmutableSet.copyOf(iterator));

Sam-Kramer · 2022-11-03T16:55:49Z

...st/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingClosableIteratorTest.java

+import java.util.function.ToLongFunction;
+import org.junit.Test;
+
+public final class TrackingClosableIteratorTest extends AbstractTrackingIteratorTest {


This would be good for the second reviewer, but in my experience I have typically avoided using abstract classes for tests except for integration/ete tests (a good example of this is the AbstractKeyValueService test, which has a strong argument).

Considering that all the methods are static and final anyways, I'd push for a utility class. It also makes the test a bit more easier to reason about.

Sam-Kramer · 2022-11-03T16:59:21Z

...a/com/palantir/atlasdb/transaction/impl/expectations/TrackingRowColumnRangeIteratorTest.java

+            Cell.create(new byte[1], new byte[1]),
+            Value.create(PtBytes.EMPTY_BYTE_ARRAY, Value.INVALID_VALUE_TIMESTAMP));
+
+    // wrapped for mocking


nit: maybe clarify that this has to be an anonymous inner class rather than a lambda in order to spy.

Sam-Kramer · 2022-11-03T17:01:30Z

...d/src/test/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIteratorTest.java

+                new TrackingIterator<>(createStringIterator(), tracker, STRING_MEASURER);
+
+        trackingIterator.forEachRemaining(string -> {
+            inOrder.verify(tracker).accept(STRING_MEASURER.applyAsLong(string));


I think this is fine, but wanted to flag as it's one of my design biases: I typically try to avoid verify unless I really have to (as it's a class out of our control, etc), what you could do here instead is simply have your track be ArrayList::add), and verify that the in-order list is equal to what you expect

Sam-Kramer · 2022-11-04T09:16:03Z

...d/src/test/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIteratorTest.java

+
+package com.palantir.atlasdb.transaction.impl.expectations;
+
+import static com.palantir.atlasdb.transaction.impl.expectations.TrackingIteratorTestUtils.STRING_MEASURER;


nit -- outside of assert4j and mockito, static imports are frowned upon for readability

Sam-Kramer · 2022-11-04T09:26:10Z

...d/src/test/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIteratorTest.java

+
+        assertThatIterator(trackingIterator)
+                .toIterable()
+                .containsExactlyElementsOf(consumeIteratorIntoList(createStringIterator()));


No need for consumeIteratorIntoList, you can just use ImmutableList.copyOf!

You can also just reference the underlying list the iterator is backed by -- imo both are acceptable.

Also we can just use assertThat which will delegate to assertThatIterator under the hood

Sam-Kramer · 2022-11-04T09:28:29Z

...d/src/test/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIteratorTest.java

+        trackingIterator.forEachRemaining(noOp());
+
+        assertThatIterable(consumed)
+                .containsExactlyElementsOf(consumeIteratorIntoList(createStringIterator()).stream()


Can just do StreamEx.of(createStringIterator()).mapToLong(STRING_MEASURER).boxed().toList() instead! IMO my preference is to separate this onto another line to make it more readable, but to be clear, this is a preference, so feel free to ignore :)

Tried to separate it into one line but my intellij did not cooperate 😅

Sam-Kramer · 2022-11-04T09:37:44Z

...st/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingClosableIteratorTest.java

+                ClosableIterators.wrapWithEmptyClose(Iterator.of(STRING)), tracker, measurer);
+
+        assertThatIterator(trackingIterator).toIterable().containsExactlyElementsOf(ImmutableSet.of(STRING));
+        trackingIterator.forEachRemaining(noOp());


This shouldn't be needed due to the above line!

...st/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingClosableIteratorTest.java

Sam-Kramer

LGTM, will wait for the other reviewer to plus one!

jeremyk-91

Generally this looks reasonable. There are a bunch of slightly unidiomatic patterns I think around mocks and the tests - it looks like you've gone through a lot of effort to avoid duplication, but the coupling between the Utils class and the resulting tests isn't really something I'm a fan of. I've scheduled some time to talk through this.

jeremyk-91 · 2022-11-08T10:34:11Z

.../test/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIteratorTestUtils.java

+
+public final class TrackingIteratorTestUtils {
+    public static final String STRING = "test";
+    // this has to be an anonymous inner class rather than lambda in order to spy


On one hand, this is a good comment!

However, I'm not sure I like this pattern as opposed to duplication, honestly. This creates a lot of coupling between the test utilities: now the utility class needs to know whether it's being spied or not, and/or if the tests no longer spy the function for some reason, how would we know that we need to also change this?

Also, while I realise this may be consistent with existing code, if this is strictly intended for use with the tracking iterators, it should be package visibility.

jeremyk-91 · 2022-11-08T11:28:12Z

.../test/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIteratorTestUtils.java

+public final class TrackingIteratorTestUtils {
+    public static final String STRING = "test";
+    // this has to be an anonymous inner class rather than lambda in order to spy
+    public static final ToLongFunction<String> STRING_MEASURER = new ToLongFunction<>() {


This should be named something different: how would I know that what's being measured is the length as opposed to something else?

jeremyk-91 · 2022-11-08T12:58:51Z

...hared/src/main/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIterator.java

+import java.util.function.Consumer;
+import java.util.function.ToLongFunction;
+
+public class TrackingIterator<T, I extends Iterator<T>> extends ForwardingIterator<T> {


Makes sense - might be good to explain what the tracker is supposed to do / give a high level overview of how this is supposed to work

jeremyk-91 · 2022-11-08T12:59:52Z

...st/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingClosableIteratorTest.java

+        assertThat(trackingIterator).toIterable().containsExactlyElementsOf(List.of(TrackingIteratorTestUtils.STRING));
+        verify(measurer, times(1)).applyAsLong(TrackingIteratorTestUtils.STRING);
+        verify(tracker, times(1))
+                .accept(TrackingIteratorTestUtils.STRING_MEASURER.applyAsLong(TrackingIteratorTestUtils.STRING));


nit: times(1) is usually not required as that's default. I'd probably also call this test method delegatesToTrackerAndMeasurer rather than IsWiredCorrectly as well.

jeremyk-91 · 2022-11-08T12:59:59Z

...st/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingClosableIteratorTest.java

+        verify(tracker, times(1))
+                .accept(TrackingIteratorTestUtils.STRING_MEASURER.applyAsLong(TrackingIteratorTestUtils.STRING));
+        verifyNoMoreInteractions(tracker);
+        verifyNoMoreInteractions(measurer);


nit: this takes varargs IIRC

...st/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingClosableIteratorTest.java

jeremyk-91 · 2022-11-08T13:01:51Z

...d/src/test/java/com/palantir/atlasdb/transaction/impl/expectations/TrackingIteratorTest.java

+        verify(tracker, times(1))
+                .accept(TrackingIteratorTestUtils.STRING_MEASURER.applyAsLong(TrackingIteratorTestUtils.STRING));
+        verifyNoMoreInteractions(tracker);
+        verifyNoMoreInteractions(measurer);


as above: I usually prefer describing behaviour as opposed to "is wired correctly", times(1) isn't needed, and vNMI takes varargs I think.

jeremyk-91 · 2022-11-08T13:02:16Z

...a/com/palantir/atlasdb/transaction/impl/expectations/TrackingRowColumnRangeIteratorTest.java

+
+    private static Value createValue(int size) {
+        Preconditions.checkArgument(size >= Long.BYTES, "size should be at least the number of bytes in one long");
+        return Value.create(new byte[size - Long.BYTES], Value.INVALID_VALUE_TIMESTAMP);


👍 appreciate the defensiveness

jeremyk-91

👍 Thanks!

jeremyk-91 · 2022-11-09T10:33:19Z

...a/com/palantir/atlasdb/transaction/impl/expectations/TrackingRowColumnRangeIteratorTest.java

@@ -57,16 +60,15 @@ public long applyAsLong(Entry<Cell, Value> value) {

    @Test
    public void oneElementTrackingRowColumnRangeIteratorIsWiredCorrectly() {
-        Consumer<Long> tracker = spy(TrackingIteratorTestUtils.noOp());
+        Consumer<Long> tracker = spy(NO_OP);


nit / non actionable: I don't normally like using constants if there is only one thing, though I'm fine either way

svc-autorelease · 2022-11-09T10:58:48Z

Released 0.759.0

adding iterators

b0f6609

Sam-Kramer reviewed Nov 2, 2022

View reviewed changes

ergo14 added 2 commits November 3, 2022 10:05

sk review flups

147ae09

use ToLongFunction instead of Function<T, Long>

442bc5d

Sam-Kramer reviewed Nov 3, 2022

View reviewed changes

sk review (2) flups

e660298

Sam-Kramer reviewed Nov 4, 2022

View reviewed changes

ergo14 added 3 commits November 4, 2022 10:50

review flups

d8fb227

test refactor

5e62e62

test refactor 2

4cc3bc2

Sam-Kramer reviewed Nov 4, 2022

View reviewed changes

ergo14 changed the title ~~[TEX] Part 1d1: TrackingKeyValueService: tracking iterator utils~~ [TEX] Part 3: TrackingKeyValueService: tracking iterator utils Nov 4, 2022

jeremyk-91 reviewed Nov 8, 2022

View reviewed changes

ergo14 and others added 3 commits November 8, 2022 22:50

pairing

0210589

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

335fc34

Add generated changelog entries

1b669d5

jeremyk-91 approved these changes Nov 9, 2022

View reviewed changes

ergo14 added autorelease merge when ready labels Nov 9, 2022

bulldozer-bot bot merged commit 4b09509 into develop Nov 9, 2022

bulldozer-bot bot deleted the tex-pr-1d1 branch November 9, 2022 10:58

ergo14 mentioned this pull request Nov 10, 2022

[TEX] Part 5: TrackingKeyValueService, tracking iterator exception handling #6361

Merged

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 3: TrackingKeyValueService: tracking iterator utils #6336

[TEX] Part 3: TrackingKeyValueService: tracking iterator utils #6336

ergo14 commented Oct 30, 2022

changelog-app bot commented Oct 30, 2022 •

edited by ergo14

Loading

Sam-Kramer left a comment

Sam-Kramer Nov 2, 2022

Sam-Kramer Nov 2, 2022

Sam-Kramer Nov 2, 2022

Sam-Kramer Nov 2, 2022

Sam-Kramer Nov 2, 2022

Sam-Kramer Nov 2, 2022 •

edited

Loading

Sam-Kramer Nov 2, 2022

Sam-Kramer Nov 2, 2022

ergo14 Nov 3, 2022

Sam-Kramer Nov 2, 2022

Sam-Kramer Nov 3, 2022

Sam-Kramer Nov 3, 2022

Sam-Kramer Nov 3, 2022

Sam-Kramer Nov 4, 2022

Sam-Kramer Nov 4, 2022

Sam-Kramer Nov 4, 2022

Sam-Kramer Nov 4, 2022

Sam-Kramer Nov 4, 2022

ergo14 Nov 4, 2022

Sam-Kramer Nov 4, 2022

Sam-Kramer left a comment

jeremyk-91 left a comment

jeremyk-91 Nov 8, 2022

jeremyk-91 Nov 8, 2022

jeremyk-91 Nov 8, 2022

jeremyk-91 Nov 8, 2022

jeremyk-91 Nov 8, 2022

jeremyk-91 Nov 8, 2022

jeremyk-91 Nov 8, 2022

jeremyk-91 left a comment

jeremyk-91 Nov 9, 2022

svc-autorelease commented Nov 9, 2022


		package com.palantir.atlasdb.transaction.impl.expectations;

		import static com.palantir.atlasdb.transaction.impl.expectations.TrackingIteratorTestUtils.STRING_MEASURER;

[TEX] Part 3: TrackingKeyValueService: tracking iterator utils #6336

[TEX] Part 3: TrackingKeyValueService: tracking iterator utils #6336

Conversation

ergo14 commented Oct 30, 2022

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

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

Sam-Kramer Nov 2, 2022 • edited Loading

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

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

Sam-Kramer left a 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

Choose a reason for hiding this comment

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

svc-autorelease commented Nov 9, 2022

changelog-app bot commented Oct 30, 2022 •

edited by ergo14

Loading

Generate changelog in `changelog/@unreleased`

Sam-Kramer Nov 2, 2022 •

edited

Loading