sweep full table #2409

nziebart · 2017-09-30T01:43:21Z

Goals (and why):
Fixes #2370

Implementation Description (bullets):

Allow specifying whether a sweep should sweep the full table (true by default)
Sweep the full table by running batches in a loop
Some refactors

Concerns (what feedback would you like?):

Where should we start reviewing?:

Priority (whenever / two weeks / yesterday):
whenever

This change is

nziebart · 2017-09-30T01:44:39Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/SweepTableResponse.java

+@JsonDeserialize(as=ImmutableSweepTableResponse.class)
+public interface SweepTableResponse {
+
+    //Optional<String> nextStartRow();


DropwizardClientRule does not use an object mapper that supports optionals so this makes all the tests fails... need to fix this

nziebart · 2017-09-30T01:45:03Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/SweepTableResponse.java

+        return ImmutableSweepTableResponse.builder()
+                .numCellTsPairsExamined(results.getCellTsPairsExamined())
+                .staleValuesDeleted(results.getStaleValuesDeleted())
+               // .nextStartRow(results.getNextStartRow().map(BaseEncoding.base16()::encode))


should extract the base16 encoding to a utility

ashrayjain · 2017-09-30T01:48:27Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/SweeperService.java

-            @Nullable @QueryParam("candidateBatchSize") Integer candidateBatchSize,
-            @Nullable @QueryParam("deleteBatchSize") Integer deleteBatchSize);
+            @QueryParam("startRow") Optional<String> startRow,
+            @QueryParam("fullSweep") @DefaultValue("true") Optional<Boolean> fullSweep,


This can be just Boolean now that we have a default value

ashrayjain · 2017-09-30T01:53:51Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/SweeperService.java

+            @QueryParam("fullSweep") @DefaultValue("true") Optional<Boolean> fullSweep,
+            @QueryParam("maxCellTsPairsToExamine") Optional<Integer> maxCellTsPairsToExamine,
+            @QueryParam("candidateBatchSize") Optional<Integer> candidateBatchSize,
+            @QueryParam("deleteBatchSize") Optional<Integer> deleteBatchSize);


Also, while we are here, can we marke these params etc. @safe for logging purposes?

codecov-io · 2017-10-03T02:03:48Z

Codecov Report

Merging #2409 into develop will increase coverage by 0.04%.
The diff coverage is 95.58%.

@@              Coverage Diff              @@
##             develop    #2409      +/-   ##
=============================================
+ Coverage      60.11%   60.15%   +0.04%     
- Complexity      3383     4650    +1267     
=============================================
  Files            857      859       +2     
  Lines          40037    40031       -6     
  Branches        4079     4076       -3     
=============================================
+ Hits           24067    24081      +14     
+ Misses         14500    14482      -18     
+ Partials        1470     1468       -2

Impacted Files	Coverage Δ	Complexity Δ
...lasdb/sweep/SweeperServiceImplIntegrationTest.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...com/palantir/atlasdb/sweep/SweepTableResponse.java	`100% <100%> (ø)`	`0 <0> (?)`
...va/com/palantir/atlasdb/sweep/SweepTaskRunner.java	`92.39% <100%> (+0.25%)`	`19 <3> (+19)`	⬆️
...ava/com/palantir/atlasdb/sweep/SweeperService.java	`100% <100%> (ø)`	`0 <0> (?)`
.../java/com/palantir/atlasdb/sweep/TableToSweep.java	`100% <100%> (ø)`	`7 <6> (+7)`	⬆️
...a/com/palantir/atlasdb/sweep/SweepBatchConfig.java	`100% <100%> (ø)`	`3 <2> (+3)`	⬆️
.../palantir/atlasdb/sweep/BackgroundSweeperImpl.java	`80.19% <100%> (-2.11%)`	`16 <0> (+16)`
...om/palantir/atlasdb/keyvalue/api/SweepResults.java	`100% <100%> (ø)`	`7 <2> (+2)`	⬆️
...m/palantir/atlasdb/sweep/SpecificTableSweeper.java	`91.2% <100%> (-1.05%)`	`23 <1> (+23)`
...com/palantir/atlasdb/sweep/SweeperServiceImpl.java	`100% <100%> (+14%)`	`1 <0> (+1)`	⬆️
... and 17 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 259290b...9bd49fa. Read the comment docs.

hsaraogi

Few small comments, approving to allow merging after fixing them!

hsaraogi · 2017-10-04T18:46:37Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/SpecificTableSweeper.java

        try {
+            log.info("Beginning");


We can add more info here.. lets have beggining sweep on table x with startrow x and batchConfig x

i added logging for this in SweepTaskRunner.java

Ohh, we will get the info from there. This line then seems a bit unnecessary.

hsaraogi · 2017-10-04T18:55:51Z

atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/sweep/SweeperServiceImplTest.java

+
+        sweeperService.sweepTable(TABLE_REF.getQualifiedName());
+
+        startRows.forEach(row -> verify(sweepTaskRunner).run(any(), any(), eq(row)));


nit: We should verify the results are not recorded here as well.

hsaraogi · 2017-10-04T19:15:42Z

atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/sweep/SweeperServiceImplTest.java

+        sweeperService.sweepTable(TABLE_REF.getQualifiedName(), Optional.empty(), Optional.of(false),
+                Optional.empty(), Optional.empty(), Optional.empty());
+
+        verify(sweepTaskRunner, times(1)).run(any(), any(), any());


nit: We should verify the results are not recorded here as well.

hsaraogi · 2017-10-04T19:16:59Z

atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/util/DropwizardClientRule.java

+
+        @Override
+        public void initialize(Bootstrap<Configuration> bootstrap) {
+            bootstrap.getObjectMapper().registerModule(new Jdk8Module());


This is non-ideal, but I couldn't think of a better way here.

hsaraogi · 2017-10-06T10:10:26Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/SweeperService.java

-    void sweepTableFromStartRow(
-            @QueryParam("tablename") String tableName,
-            @Nonnull @QueryParam("startRow") String startRow);
+    default SweepTableResponse sweepTable(String tableName) {


should we get rid of these APIs as we are no longer exposing the endpoints. This is technically a break, but probably noone will be affected.

i mainly just added these for easier usage in java (otherwise you have to pass in 5 Optional.empty()s

fsamuel-bs · 2017-10-06T13:04:08Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/SweepTaskRunner.java

@@ -140,6 +142,10 @@ private SweepResults doRun(TableReference tableRef,
                               byte[] startRow,
                               RunType runType,
                               Sweeper sweeper) {
+        log.info("Beginning iteration of sweep for table {} starting at row {}",


If we have this logging, do we want to remove the loggings on BackgroundSweeperImpl - the ones on getTableToSweep? Otherwise we'd log the same thing twice (that we're starting to sweep table {} on row {}).

You also probably want to update the sweep-logs.rst document to keep track of where we log this line.

oh i see, we already having logging there. ok, i'll just move this to the SweeperService

actually, going to leave this and delete the other, as otherwise we'd have to log in multiple places

hsaraogi · 2017-10-10T10:37:57Z

atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/sweep/SweeperServiceImplTest.java

-        Mockito.verify(priorityStore, never()).update(Mockito.any(), Mockito.any(), Mockito.any());
-        Mockito.verify(progressStore, never()).saveProgress(Mockito.any(), Mockito.any());
+        sweeperService.sweepTableFrom(TABLE_REF.getQualifiedName(), encodeStartRow(new byte[] {1, 2, 3}));
+        verify(priorityStore, never()).update(any(), any(), any());


We can also delete this test as we are verifying no metrics for all tests, but will not block on this.

fsamuel-bs

Pinging @nziebart to update the sweep-logs.rst docs before merging this. If not on this PR, you can also do it shortly after, and you can dismiss this review.

nziebart · 2017-10-11T23:44:48Z

@fsamuel-bs already done :)

nziebart · 2017-10-16T13:02:04Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/SweeperService.java

-     * Sweep a particular table from specified startRow with specified {@link SweepBatchConfig} parameters.
+     * Sweeps a particular table.
+     *
+     * @param tableName the table to sweep


add docs for format of table and row

nziebart · 2017-10-16T16:19:59Z

@fsamuel-bs are you good with the docs changes here?

fsamuel-bs

LGTM.

…weep-full-table

nziebart requested review from fsamuel-bs and hsaraogi September 30, 2017 01:43

nziebart commented Sep 30, 2017

View reviewed changes

ashrayjain reviewed Sep 30, 2017

View reviewed changes

hsaraogi approved these changes Oct 4, 2017

View reviewed changes

Nathan Ziebart added 5 commits October 5, 2017 19:16

sweep full table

cabb472

fix tests

c13f5e8

logsafe

391aeb6

release notes

27d3a3f

pr comments

e40704c

nziebart force-pushed the nziebart/sweep-full-table branch from 08c89ed to e40704c Compare October 6, 2017 02:18

hsaraogi reviewed Oct 6, 2017

View reviewed changes

fsamuel-bs reviewed Oct 6, 2017

View reviewed changes

Nathan Ziebart added 2 commits October 6, 2017 18:21

docs

b070b1a

unnecessary method call

ac4e1a9

hsaraogi reviewed Oct 10, 2017

View reviewed changes

hsaraogi approved these changes Oct 10, 2017

View reviewed changes

checkstyle

5cef9c5

fsamuel-bs suggested changes Oct 11, 2017

View reviewed changes

nziebart and others added 3 commits October 11, 2017 21:41

Merge branch 'develop' into nziebart/sweep-full-table

00c7e2f

fix build

b6fd39c

fix build

ee92057

hsaraogi assigned nziebart Oct 16, 2017

nziebart commented Oct 16, 2017

View reviewed changes

nziebart and others added 2 commits October 16, 2017 16:33

Merge branch 'develop' into nziebart/sweep-full-table

ce46686

javadocs

4e60f45

nziebart assigned fsamuel-bs and unassigned nziebart Oct 16, 2017

fsamuel-bs approved these changes Oct 17, 2017

View reviewed changes

fsamuel-bs added 2 commits October 17, 2017 14:20

Merge branch 'develop' of github.com:palantir/atlasdb into nziebart/s…

2ee81e2

…weep-full-table

Random logging bits

9bd49fa

fsamuel-bs merged commit 13a2b81 into develop Oct 17, 2017

fsamuel-bs deleted the nziebart/sweep-full-table branch October 17, 2017 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sweep full table #2409

sweep full table #2409

nziebart commented Sep 30, 2017 •

edited by jboreiko

Loading

nziebart Sep 30, 2017

nziebart Sep 30, 2017

ashrayjain Sep 30, 2017

ashrayjain Sep 30, 2017

codecov-io commented Oct 3, 2017 •

edited

Loading

hsaraogi left a comment

hsaraogi Oct 4, 2017

nziebart Oct 6, 2017

hsaraogi Oct 6, 2017

hsaraogi Oct 4, 2017

nziebart Oct 10, 2017

hsaraogi Oct 4, 2017

nziebart Oct 10, 2017

hsaraogi Oct 4, 2017

hsaraogi Oct 6, 2017

nziebart Oct 10, 2017

fsamuel-bs Oct 6, 2017

nziebart Oct 7, 2017

nziebart Oct 7, 2017

hsaraogi Oct 10, 2017 •

edited

Loading

fsamuel-bs left a comment

nziebart commented Oct 11, 2017

nziebart Oct 16, 2017

nziebart commented Oct 16, 2017

fsamuel-bs left a comment


		sweeperService.sweepTable(TABLE_REF.getQualifiedName());

		startRows.forEach(row -> verify(sweepTaskRunner).run(any(), any(), eq(row)));

sweep full table #2409

sweep full table #2409

Conversation

nziebart commented Sep 30, 2017 • edited by jboreiko 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

codecov-io commented Oct 3, 2017 • edited Loading

Codecov Report

hsaraogi 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

Choose a reason for hiding this comment

hsaraogi Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

fsamuel-bs left a comment

Choose a reason for hiding this comment

nziebart commented Oct 11, 2017

Choose a reason for hiding this comment

nziebart commented Oct 16, 2017

fsamuel-bs left a comment

Choose a reason for hiding this comment

nziebart commented Sep 30, 2017 •

edited by jboreiko

Loading

codecov-io commented Oct 3, 2017 •

edited

Loading

hsaraogi Oct 10, 2017 •

edited

Loading