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

Actually log batch parameters in BackgroundSweeperImpl and friends #2444

Closed
wants to merge 4 commits into from

Conversation

jeremyk-91
Copy link
Contributor

@jeremyk-91 jeremyk-91 commented Oct 6, 2017

Goals (and why):

  • There's a VERY annoying issue where in sweep logs the batch is printed as {}. This fixes that and returns the three values (candidate batch size, delete batch size, max no. of cell-ts pairs to examine)

Implementation Description (bullets):

  • Log the values separately
  • Release notes

Concerns (what feedback would you like?):

  • nothing in particular.

Where should we start reviewing?: BackgroundSweeperImpl

Priority (whenever / two weeks / yesterday): soon, please - makes debugging sweep by sniffing its logs very hard


This change is Reviewable

Copy link
Contributor

@hsaraogi hsaraogi left a comment

Choose a reason for hiding this comment

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

LGTM, confirmed that the logging spec doesnt call toString on objects.

@@ -150,7 +150,9 @@ private long grabLocksAndRun(SweepLocks locks) throws InterruptedException {
SweepBatchConfig lastBatchConfig = getAdjustedBatchConfig();
log.warn("The background sweep job failed unexpectedly with batch config {}."
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't this line and the number of {} need to have an equivalent change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, of course. I think I got too excited when I saw that the json params were logged correctly...

@codecov-io
Copy link

codecov-io commented Oct 9, 2017

Codecov Report

Merging #2444 into develop will decrease coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2444      +/-   ##
=============================================
- Coverage      60.09%   60.08%   -0.02%     
+ Complexity      4522     3213    -1309     
=============================================
  Files            833      833              
  Lines          39414    39395      -19     
  Branches        4037     4036       -1     
=============================================
- Hits           23687    23671      -16     
+ Misses         14253    14250       -3     
  Partials        1474     1474
Impacted Files Coverage Δ Complexity Δ
...m/palantir/atlasdb/sweep/SpecificTableSweeper.java 92.24% <0%> (-1.46%) 0 <0> (-23)
.../palantir/atlasdb/sweep/BackgroundSweeperImpl.java 79.64% <100%> (+0.36%) 0 <0> (-18) ⬇️
...ain/java/com/palantir/paxos/PaxosProposerImpl.java 85% <0%> (-6.67%) 0% <0%> (ø)
...ain/java/com/palantir/paxos/PaxosAcceptorImpl.java 69.64% <0%> (-3.58%) 0% <0%> (ø)
...db/keyvalue/cassandra/CassandraClientPoolImpl.java 44.07% <0%> (-1.3%) 0% <0%> (ø)
...asdb/transaction/impl/SerializableTransaction.java 87.85% <0%> (-0.57%) 0% <0%> (-86%)
.../atlasdb/transaction/impl/SnapshotTransaction.java 72.04% <0%> (-0.57%) 0% <0%> (-182%)
...lantir/atlasdb/keyvalue/cassandra/CqlExecutor.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...n/java/com/palantir/lock/impl/LockServiceImpl.java 82.95% <0%> (+0.35%) 90% <0%> (ø) ⬇️
...om/palantir/leader/PaxosLeaderElectionService.java 81.92% <0%> (+0.38%) 0% <0%> (ø) ⬇️
... and 2 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 5f828fa...5b8a28a. Read the comment docs.

@fsamuel-bs
Copy link
Contributor

Closing in favor of #2475.

@fsamuel-bs fsamuel-bs closed this Oct 11, 2017
fsamuel-bs added a commit that referenced this pull request Oct 11, 2017
fsamuel-bs added a commit that referenced this pull request Oct 17, 2017
* Better logging for internal customer

* Update docs

* Copy over #2444
@felixdesouza felixdesouza deleted the jkong/bsi-logging branch July 2, 2019 22:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants