Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

SweeperService logging improvements #2618

Merged
merged 8 commits into from
Nov 8, 2017
Merged

SweeperService logging improvements #2618

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c3376ab
SweeperServiceImpl now logs when it starts sweeping make it clear if …
tboam Nov 2, 2017
b067262
Added sweep parameters to the log lines
tboam Nov 3, 2017
1b649ea
Merge branch 'develop' into logs/sweep-service-logging
tboam Nov 6, 2017
19239bc
Fix release notes
fsamuel-bs Nov 6, 2017
606617e
Merge branch 'develop' into logs/sweep-service-logging
tboam Nov 6, 2017
c14c7b7
no longer default the service parameter in the interface, this way we…
tboam Nov 7, 2017
3b22141
Merge branch 'develop' into logs/sweep-service-logging
tboam Nov 7, 2017
3b89c73
Checkstyle
fsamuel-bs Nov 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 38 additions & 11 deletions atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/SweeperServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,21 @@

import java.util.Optional;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.google.common.base.Preconditions;
import com.palantir.atlasdb.encoding.PtBytes;
import com.palantir.atlasdb.keyvalue.api.SweepResults;
import com.palantir.atlasdb.keyvalue.api.TableReference;
import com.palantir.atlasdb.logging.LoggingArgs;
import com.palantir.logsafe.SafeArg;
import com.palantir.logsafe.UnsafeArg;
import com.palantir.remoting3.servers.jersey.WebPreconditions;

public final class SweeperServiceImpl implements SweeperService {
private static final Logger log = LoggerFactory.getLogger(SweeperServiceImpl.class);

private final SpecificTableSweeper specificTableSweeper;
private final AdjustableSweepBatchConfigSource sweepBatchConfigSource;

Expand All @@ -48,17 +56,36 @@ public SweepTableResponse sweepTable(
SweepBatchConfig config = buildConfigWithOverrides(maxCellTsPairsToExamine, candidateBatchSize,
deleteBatchSize);

SweepResults sweepResults = fullSweep.orElse(true)
? runFullSweepWithoutSavingResults(
tableRef,
decodedStartRow,
config)
: runOneBatchWithoutSavingResults(
tableRef,
decodedStartRow,
config);

return SweepTableResponse.from(sweepResults);
return SweepTableResponse.from(runSweep(fullSweep, tableRef, decodedStartRow, config));
}

private SweepResults runSweep(Optional<Boolean> fullSweep, TableReference tableRef, byte[] decodedStartRow,
SweepBatchConfig config) {
if (!fullSweep.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will be ever triggered, as we have the @Safe @QueryParam("fullSweep") @DefaultValue("true") Optional<Boolean> fullSweep on the interface. Still, it doesn't seem harmless — the only drawback is that when changing the default, we probably want to change it in two places, which are not that far apart.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is irritating - there's no way to distinguish when the value was filled in by the web container.

Ho would you feel about removing the @Default annotation? The default is still to true and documentation is unchanged, but then the defaulting no longer happens near where it is documented which is going to cause them to get out of sync.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to log this at warn when we have the below info logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to call attention to this if it happens - people shouldn't be using this endpoint in normal operation and there's a good chance it was called by mistake.

log.warn("fullSweep parameter was not specified, defaulting to true");
}

if (fullSweep.orElse(true)) {
log.info("Running sweep of full table {}, "
+ "with maxCellTsPairsToExamine: {}, candidateBatchSize: {}, deleteBatchSize: {}, "
+ "starting from row {}",
LoggingArgs.tableRef(tableRef),
SafeArg.of("maxCellTsPairsToExamine", config.maxCellTsPairsToExamine()),
SafeArg.of("candidateBatchSize", config.candidateBatchSize()),
SafeArg.of("deleteBatchSize", config.deleteBatchSize()),
UnsafeArg.of("decodedStartRow", decodedStartRow));
return runFullSweepWithoutSavingResults(tableRef, decodedStartRow, config);
} else {
log.info("Running sweep of a single batch on table {}, "
+ "with maxCellTsPairsToExamine: {}, candidateBatchSize: {}, deleteBatchSize: {}, "
+ "starting from row {}",
LoggingArgs.tableRef(tableRef),
SafeArg.of("maxCellTsPairsToExamine", config.maxCellTsPairsToExamine()),
SafeArg.of("candidateBatchSize", config.candidateBatchSize()),
SafeArg.of("deleteBatchSize", config.deleteBatchSize()),
UnsafeArg.of("decodedStartRow", decodedStartRow));
return runOneBatchWithoutSavingResults(tableRef, decodedStartRow, config);
}
}

private SweepBatchConfig buildConfigWithOverrides(
Expand Down
6 changes: 6 additions & 0 deletions docs/source/release_notes/release-notes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ develop

* - Type
- Change

* -
-

* - |improved| |logs|
- ``SweeperServiceImpl`` now logs when it starts sweeping and makes it clear if it is running full sweep or not

* - |improved| |metrics|
- AtlasDB now depends on Tritium 0.8.0, allowing products to upgrade Tritium without running into `NoClassDefFound` errors.
Expand Down