[improvement] Refactor Auditor to simplify the readability

[wenbingshen]

Motivation

Fix #3636

See explanation in #3636

Changes

1. Auditor class: Only reserved to do some necessary initialization work, scheduling of various detection tasks
2. Separate monitoring metrics into AuditorStats
3. Separate the auditBookies task to the AuditorBookieCheckTask
4. Separate the checkAllLedgers task into the AuditorCheckAllLedgersTask
5. Separate placementPolicyCheck task into AuditorPlacementPolicyCheckTask
6. Separate replicasCheck to AuditorReplicasCheckTask

[wenbingshen]

rerun failure checks

[eolivelli]

+1
thanks for this improvement, it will help a lot new contributions

[wenbingshen]

ping @zymap @hangc0276 @shoothzj @dlg99 If you have time, can you help take a look. Thanks.

[wenbingshen]

rerun failure checks

[wenbingshen]

@hangc0276 I have addressed all your comments. PTAL.

[dlg99]

First of all, thank you for taking on this task.
I recognize it is not simple, given all the iterations of this code and "small changes" snuck there over the years.

Looking at this version I feel it fails a bit short of original plan of refactoring.
It does split the code into smaller components but it leaves the components interconnected - tasks that have to access Auditor's internals etc. Typically this is a design smell that will backfire in the future when changing the code or it will become a burden when you start writing unit tests and mocks for the tasks
Second point is the unit tests. I think that new tasks deserve some unit tests.

This is a fantastic job so far and I applaud you for doing it.
I hope my comment makes sense and it can be made even better.

[wenbingshen]

I hope my comment makes sense and it can be made even better.

@dlg99 Thanks for all your comments. The comments make a lot of sense, I'll try to perfect the whole PR.

[wenbingshen]

It does split the code into smaller components but it leaves the components interconnected - tasks that have to access Auditor's internals etc. Typically this is a design smell that will backfire in the future when changing the code or it will become a burden when you start writing unit tests and mocks for the tasks
Second point is the unit tests. I think that new tasks deserve some unit tests.

@dlg99

1. I updated the code to avoid circular dependency between Auditor and CheckTask.
2. I added unit tests corresponding to different tasks.
3. Now these tasks can run independently without Auditor.
4. These tasks all have basic unit tests, indicating that they can run independently without Auditor. Since there are too many items that can be used for testing, if we want other unit tests to cover more options, I hope to use another PR. The previous unit tests are sufficient to prove that the changes in this PR maintain compatibility.

If tests not fail, PTAL. Thanks!

[dlg99]

LGTM, thank you!

One test failed, I am re-running the test suite, we'll see if that's a flake.

[wenbingshen]

I have another bugfix for Auditor. I want to commit after this PR so that I can avoid rebase master and resolve code conflicts. Can you help me to see this PR? Thank you very much. @hangc0276 @dlg99 @eolivelli

[codecov-commenter]

Codecov Report

Merging #3637 (2cd8a1e) into master (d6d9212) will increase coverage by 9.93%.
The diff coverage is 70.82%.

@@             Coverage Diff              @@
##             master    #3637      +/-   ##
============================================
+ Coverage     58.23%   68.17%   +9.93%     
- Complexity     5612     6712    +1100     
============================================
  Files           467      473       +6     
  Lines         40822    40943     +121     
  Branches       5234     5234              
============================================
+ Hits          23774    27913    +4139     
+ Misses        14933    10768    -4165     
- Partials       2115     2262     +147     

Flag	Coverage Δ
bookie	39.68% <0.00%> (?)
client	44.28% <20.88%> (+0.04%)	⬆️
remaining	29.61% <70.82%> (+0.11%)	⬆️
replication	41.41% <24.51%> (+0.01%)	⬆️
tls	21.08% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ava/org/apache/bookkeeper/replication/Auditor.java	74.26% <ø> (+4.57%)	⬆️
...keeper/replication/AuditorCheckAllLedgersTask.java	56.46% <56.46%> (ø)
...r/replication/AuditorPlacementPolicyCheckTask.java	64.63% <64.63%> (ø)
...okkeeper/replication/AuditorReplicasCheckTask.java	73.78% <73.78%> (ø)
...bookkeeper/replication/AuditorBookieCheckTask.java	75.67% <75.67%> (ø)
...rg/apache/bookkeeper/replication/AuditorStats.java	82.14% <82.14%> (ø)
...org/apache/bookkeeper/replication/AuditorTask.java	88.09% <88.09%> (ø)
...rg/apache/bookkeeper/proto/ReadLacProcessorV3.java	73.33% <0.00%> (-5.00%)	⬇️
.../main/java/org/apache/bookkeeper/util/ZkUtils.java	82.47% <0.00%> (-4.13%)	⬇️
...apache/bookkeeper/client/ReadOnlyLedgerHandle.java	83.21% <0.00%> (-2.10%)	⬇️
... and 119 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[horizonzy]

LGTM.

[hangc0276]

Overall LGTM.