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

(WIP) Auditor.checkAllLedgers should use Auditor's instance variables #1588

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reddycharan
Copy link
Contributor

Descriptions of the changes in this PR:

  • currently checkAllLedgers method creates instances of ZK, BK and BKAdmin
    everytime this method is called. Instead it should use Auditors instance
    variables.

- currently checkAllLedgers method creates instances of ZK, BK and BKAdmin
everytime this method is called. Instead it should use Auditors instance
variables.
@reddycharan
Copy link
Contributor Author

retest this please

@reddycharan
Copy link
Contributor Author

few replication tests are failing. Will check that.

@reddycharan reddycharan changed the title Auditor.checkAllLedgers should use Auditor's instance variables (WIP) Auditor.checkAllLedgers should use Auditor's instance variables Aug 9, 2018
@sijie
Copy link
Member

sijie commented Aug 22, 2018

the change here will introduce deadlock so this PR needs to be revisited.

See comment:
#1608 (comment)

@eolivelli
Copy link
Contributor

@reddycharan this patch got enough consensus to be committed.
can you please rebase ?
I will be happy to merge

cc @sijie

@eolivelli
Copy link
Contributor

Oh I missed @sijie 's comment. we should revisit this patch or close it

@eolivelli
Copy link
Contributor

@reddycharan this patch was already approved by @sijie

do you mind rebasing it to current master ?

@Ghatage
Copy link
Contributor

Ghatage commented Oct 28, 2020

@reddycharan can you please rebase to master and push? This is a useful patch to have.

@Ghatage
Copy link
Contributor

Ghatage commented Jul 12, 2024

@eolivelli @sijie I went through the current implementation of checkAllLedgers() and I think we should close this PR.

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.

4 participants