Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix wrong update checkAllLedgersTime when ledgerReplication disabled #3939

Merged

Conversation

wenbingshen
Copy link
Member

@wenbingshen wenbingshen commented Apr 26, 2023

Motivation

When ledgerReplication disabled, we don't need to register checkAllLedgersTime failedEvent because we already skipped the check and didn't encounter any exception.

        Stopwatch stopwatch = Stopwatch.createStarted();
        boolean checkSuccess = false;
        try {
            if (!isLedgerReplicationEnabled()) {
                LOG.info("Ledger replication disabled, skipping checkAllLedgers");
                checkSuccess = true;    <= here
                return;
            }

            LOG.info("Starting checkAllLedgers");
            checkAllLedgers();
            ......
            checkSuccess = true;
        } catch (InterruptedException ie) {
           ......
        } finally {
            if (!checkSuccess) {
                long checkAllLedgersDuration = stopwatch.stop().elapsed(TimeUnit.MILLISECONDS);
                auditorStats.getCheckAllLedgersTime()
                        .registerFailedEvent(checkAllLedgersDuration, TimeUnit.MILLISECONDS);
            }
        }

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

good catch !

no need to add a test in this case

@wenbingshen
Copy link
Member Author

rerun failure checks

@hangc0276 hangc0276 added this to the 4.17.0 milestone Apr 27, 2023
@hangc0276 hangc0276 merged commit c765aea into apache:master Apr 27, 2023
16 checks passed
@wenbingshen wenbingshen deleted the wenbing/fixWrongUpdateCheckLedgersTime branch April 27, 2023 05:43
zymap pushed a commit that referenced this pull request Jun 19, 2023
…3939)

### Motivation

When ledgerReplication disabled, we don't need to register `checkAllLedgersTime` failedEvent because we already skipped the check and didn't encounter any exception.

```java
        Stopwatch stopwatch = Stopwatch.createStarted();
        boolean checkSuccess = false;
        try {
            if (!isLedgerReplicationEnabled()) {
                LOG.info("Ledger replication disabled, skipping checkAllLedgers");
                checkSuccess = true;    <= here
                return;
            }

            LOG.info("Starting checkAllLedgers");
            checkAllLedgers();
            ......
            checkSuccess = true;
        } catch (InterruptedException ie) {
           ......
        } finally {
            if (!checkSuccess) {
                long checkAllLedgersDuration = stopwatch.stop().elapsed(TimeUnit.MILLISECONDS);
                auditorStats.getCheckAllLedgersTime()
                        .registerFailedEvent(checkAllLedgersDuration, TimeUnit.MILLISECONDS);
            }
        }
```

(cherry picked from commit c765aea)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants