Issue 2570: Update segment metrics when a transaction merge happens #3239

kevinhan88 · 2019-01-04T10:31:47Z

Signed-off-by: kevinhan88 kevinhan88@gmail.com

Change log description
Transaction segment write_bytes and write_events will only be added into parent segment metrics when transaction segments merge happens.

Purpose of the change
Fixes #2570

What the code does
Inside StreamSegmentContainer.java when mergeStreamSegment() is called, if the sourceSegment is not empty and is a transaction segment, then:
.the length of source segment is added into the WRITE_BYTES metrics of target segment
.the event count of source segment is added into the WRITE_EVENTS metrics of target segment

How to verify it
Integration unit test included.
Can also be observed in live metrics: after txn commit, the metrics of parent segment changes; if txn is aborted, no change to the parent segment.

RaulGracia

Travis build is failing (StreamSegmentContainerTests).

codecov · 2019-01-04T22:07:08Z

Codecov Report

Merging #3239 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #3239      +/-   ##
============================================
- Coverage     78.84%   78.81%   -0.03%     
+ Complexity     8016     8015       -1     
============================================
  Files           609      609              
  Lines         31476    31479       +3     
  Branches       3041     3042       +1     
============================================
- Hits          24816    24810       -6     
- Misses         4870     4876       +6     
- Partials       1790     1793       +3

Impacted Files	Coverage Δ	Complexity Δ
...mentstore/server/host/handler/AppendProcessor.java	`76.14% <100%> (+0.5%)`	`34 <0> (+1)`	⬆️
...e/server/host/handler/PravegaRequestProcessor.java	`71.81% <100%> (+2.39%)`	`65 <0> (+3)`	⬆️
...ntstore/server/reading/StorageReadResultEntry.java	`66.66% <0%> (-25%)`	`2% <0%> (ø)`
...main/java/io/pravega/controller/task/TaskBase.java	`77.57% <0%> (-6.55%)`	`26% <0%> (-3%)`
...a/io/pravega/controller/metrics/StreamMetrics.java	`74.13% <0%> (-5.18%)`	`10% <0%> (-1%)`
...erver/logs/operations/CheckpointOperationBase.java	`90.9% <0%> (-4.55%)`	`4% <0%> (-1%)`
.../client/stream/impl/ControllerResolverFactory.java	`82.56% <0%> (-3.67%)`	`7% <0%> (ø)`
...eventProcessor/requesthandlers/SealStreamTask.java	`90.9% <0%> (-3.64%)`	`23% <0%> (-1%)`
...ega/controller/store/task/ZKTaskMetadataStore.java	`82.6% <0%> (-2.9%)`	`12% <0%> (ø)`
...ga/controller/store/client/StoreClientFactory.java	`79.59% <0%> (-2.05%)`	`10% <0%> (-1%)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 289d02d...bc73bda. Read the comment docs.

RaulGracia · 2019-01-07T07:51:33Z

...e/server/src/main/java/io/pravega/segmentstore/server/containers/StreamSegmentContainer.java

@@ -493,6 +504,13 @@ public boolean isOffline() {
            // the Merge right after the Seal.
            result = CompletableFuture.allOf(result,
                    this.durableLog.add(new MergeSegmentOperation(targetSegmentId, sourceSegmentId), timer.getRemaining()));
+            //if segment is a transaction, add its length onto metrics of target segment


It is a good idea to place this kind of metrics logic in upper layers whenever possible. For this reason, it would be nice to explore if it is possible to report these metrics in PravegaRequestProcessor.mergeSegments (there is a private method recordStatForTransaction in which you can add your reporting logic).

andreipaduroiu · 2019-01-07T21:58:50Z

I am OK with this, but just FYI that we use that metric to measure ingestion throughput. Since the data for transactions has already been reported, we will get throughput spikes for every transaction merge, even if we did not actually ingest any data.

kevinhan88 · 2019-01-07T22:18:21Z

@andreipaduroiu yes this approach will cause spike for segment level throughput. Hopefully the situation is mitigated by the global WRITE_BYTES and WRITE_EVENTS metrics as these two are reported regardless.

RaulGracia

LGTM (just a couple of minor comments).

RaulGracia · 2019-01-10T20:07:49Z

test/integration/src/test/java/io/pravega/test/integration/TransactionMetricsTest.java

+@Slf4j
+public class TransactionMetricsTest {
+
+  private final int controllerPort = TestUtils.getAvailableListenPort();


Minor: indentation in this class seems to be the half of what it should be, right?

RaulGracia · 2019-01-10T20:18:54Z

.../host/src/main/java/io/pravega/segmentstore/server/host/handler/PravegaRequestProcessor.java

@@ -623,6 +625,13 @@ private void recordStatForTransaction(SegmentProperties sourceInfo, String targe
                if (statsRecorder != null) {
                    statsRecorder.merge(targetSegmentName, len, numOfEvents, creationTime);
                }
+
+                //if source segment is a transaction, add its length and event count onto metrics of target segment


Please, according to our guidelines, comments start by space and capitals.

Signed-off-by: kevinhan88 <kevinhan88@gmail.com>

…since some ad-hoc code may skip it Signed-off-by: kevinhan88 <kevinhan88@gmail.com>

Signed-off-by: kevinhan88 <kevinhan88@gmail.com>

andreipaduroiu · 2019-01-11T02:05:09Z

...t/src/test/java/io/pravega/segmentstore/server/host/handler/PravegaRequestProcessorTest.java

+        field.set(null, newValue);
+    }
+
+    SegmentProperties createSegmentProperty(String streamSegmentName, UUID txnId) {


Use StreamSegmentInformation.builder(). …. build()

andreipaduroiu · 2019-01-11T02:06:26Z

test/integration/src/test/java/io/pravega/test/integration/TransactionMetricsTest.java

+        statsProvider.start();
+        log.info("Metrics Stats provider is started");
+
+        executor = Executors.newSingleThreadScheduledExecutor();


Derive this class from ThreadPooledTestSuite and override getThreadPoolSize. That will take care of creating executors and properly shutting them down for you.

This entire test has been replaced by light weight mock tests.

andreipaduroiu · 2019-01-11T02:07:36Z

test/integration/src/test/java/io/pravega/test/integration/TransactionMetricsTest.java

+                updatedCounter = true;
+            } catch (AssertionError e) {
+                log.info("Metric not updated in the cache. Retrying.", e);
+                Exceptions.handleInterrupted(() -> Thread.sleep(100));


Is there no better way of doing this than using a busy-wait loop?

You're right, we shouldn't have too many unit integration tests - as of today it already takes over 40 minutes to finish all unit tests.
I've deleted this integration test, and added some mock tests.

RaulGracia · 2019-01-11T07:53:21Z

...t/src/test/java/io/pravega/segmentstore/server/host/handler/PravegaRequestProcessorTest.java

@@ -1,20 +1,23 @@
 /**
 * Copyright (c) 2017 Dell Inc., or its subsidiaries. All Rights Reserved.
- *
+ * <p>


Please, remove these markers from the license header.

Signed-off-by: kevinhan88 <kevinhan88@gmail.com>

* master: (31 commits) Issue 3279: Client is performing blocking operations inside Netty Event Loop (pravega#3280) Issue 3200: Document Review: wire-protocol.md (pravega#3201) Issue 2933: (SegmentStore) testEndToEndWithFencing fixes (pravega#3294) Issue pravega#2768: Rename PravegaCredsWrapper class and its member variable (pravega#3302) Issue pravega#3299: Reduce dependencies in client (pravega#3300) Issue 2895: Wire Protocol implementation for TableStore (pravega#3283) Issue-3239: Make DYNAMIC_LOGGER non-static (pravega#3293) Issue 2956: (SegmentStore) TableSegment.deleteIfEmpty (pravega#3263) Issue pravega#3289: Use compile-time dependencies where possible (pravega#3290) Issue 3223: RetentionTest failure (pravega#3272) Issue 2001: Make a defensive copy of streamCut's position map (pravega#3281) Issue 3287: Exclude "passwd" extensions from rat. (pravega#3288) Issue 3285: Changed image pull policy. (pravega#3286) Issue 3266: Ensure bookkeeper.bkAckQuorumSize is 3 for BookieFailover test. (pravega#3267) Issue 3264: Return a SegmentHandle from Storage create method. (pravega#3276) Issue 3086: Controller Diagrams(new) included (pravega#3097) Issue 2570: Update segment metrics when a transaction merge happens (pravega#3239) Issue 3149: (SegmentStore) Pinned Segments (pravega#3215) Issue 3269: Fix invalid initialization order in AbstractScaleTests. (pravega#3270) Issue 3148: (Segment Store) Segment Metadata Store (pravega#3206) ...

fpj requested a review from RaulGracia January 4, 2019 10:37

RaulGracia requested changes Jan 4, 2019

View reviewed changes

kevinhan88 force-pushed the issue-2570 branch from 9fe449b to 87d7b8a Compare January 4, 2019 21:20

RaulGracia requested changes Jan 7, 2019

View reviewed changes

kevinhan88 force-pushed the issue-2570 branch 3 times, most recently from 522e94e to 2107d98 Compare January 8, 2019 18:03

fpj changed the title ~~Issue-2570: Update segment metrics when a transaction merge happens~~ Issue 2570: Update segment metrics when a transaction merge happens Jan 9, 2019

RaulGracia approved these changes Jan 10, 2019

View reviewed changes

kevinhan88 force-pushed the issue-2570 branch from 04f28a0 to f95f023 Compare January 10, 2019 22:54

kevinhan88 added 4 commits January 10, 2019 16:18

Issue-2570: Update segment metrics when a transaction merge happens

5a60596

Signed-off-by: kevinhan88 <kevinhan88@gmail.com>

Issue-2570: added null pointer check for segment metadata attributes …

0eaa4cb

…since some ad-hoc code may skip it Signed-off-by: kevinhan88 <kevinhan88@gmail.com>

Issue-2570: moved metrics logic to higher level for better control

e22f49c

Signed-off-by: kevinhan88 <kevinhan88@gmail.com>

Issue-2570: improved code coverage by adding more unit tests

51824c7

Signed-off-by: kevinhan88 <kevinhan88@gmail.com>

kevinhan88 force-pushed the issue-2570 branch from f95f023 to df25e71 Compare January 11, 2019 00:20

Issue-2570: addressed code style comments

387c0b7

Signed-off-by: kevinhan88 <kevinhan88@gmail.com>

kevinhan88 force-pushed the issue-2570 branch from df25e71 to 387c0b7 Compare January 11, 2019 00:57

andreipaduroiu requested changes Jan 11, 2019

View reviewed changes

RaulGracia reviewed Jan 11, 2019

View reviewed changes

kevinhan88 force-pushed the issue-2570 branch from a068d50 to 57cb508 Compare January 11, 2019 08:35

Issue-2570: replaced the integration test with mock test

bc73bda

Signed-off-by: kevinhan88 <kevinhan88@gmail.com>

kevinhan88 force-pushed the issue-2570 branch from 57cb508 to bc73bda Compare January 11, 2019 08:42

andreipaduroiu approved these changes Jan 11, 2019

View reviewed changes

andreipaduroiu merged commit 38a466a into pravega:master Jan 11, 2019

tkaitchuck mentioned this pull request Jan 17, 2019

Sporadic unit test failure testAppendFails #3292

Closed

kevinhan88 mentioned this pull request Jan 18, 2019

Issue-3239: make DYNAMIC_LOGGER non-static to avoid test code race condition #3293

Merged

kevinhan88 deleted the issue-2570 branch June 1, 2020 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 2570: Update segment metrics when a transaction merge happens #3239

Issue 2570: Update segment metrics when a transaction merge happens #3239

kevinhan88 commented Jan 4, 2019 •

edited

RaulGracia left a comment

codecov bot commented Jan 4, 2019 •

edited

RaulGracia Jan 7, 2019

kevinhan88 Jan 7, 2019

andreipaduroiu commented Jan 7, 2019

kevinhan88 commented Jan 7, 2019

RaulGracia left a comment

RaulGracia Jan 10, 2019

kevinhan88 Jan 10, 2019

RaulGracia Jan 10, 2019

kevinhan88 Jan 10, 2019

andreipaduroiu Jan 11, 2019

kevinhan88 Jan 11, 2019

andreipaduroiu Jan 11, 2019

kevinhan88 Jan 11, 2019

andreipaduroiu Jan 11, 2019

kevinhan88 Jan 11, 2019

RaulGracia Jan 11, 2019

kevinhan88 Jan 11, 2019

Issue 2570: Update segment metrics when a transaction merge happens #3239

Issue 2570: Update segment metrics when a transaction merge happens #3239

Conversation

kevinhan88 commented Jan 4, 2019 • edited

RaulGracia left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 4, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreipaduroiu commented Jan 7, 2019

kevinhan88 commented Jan 7, 2019

RaulGracia 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

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

kevinhan88 commented Jan 4, 2019 •

edited

codecov bot commented Jan 4, 2019 •

edited