Skip to content

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

Open
reddycharan wants to merge 1 commit into
apache:masterfrom
reddycharan:checkallledgersfix
Open

reddycharan wants to merge 1 commit into
apache:masterfrom
reddycharan:checkallledgersfix

Conversation

@reddycharan

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

retest this please

@reddycharan

Copy link
Copy Markdown
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

sijie commented Aug 22, 2018

Copy link
Copy Markdown
Member

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

See comment:
#1608 (comment)

@eolivelli

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@eolivelli

Copy link
Copy Markdown
Contributor

@reddycharan this patch was already approved by @sijie

do you mind rebasing it to current master ?

@Ghatage

Ghatage commented Oct 28, 2020

Copy link
Copy Markdown
Contributor

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

@Ghatage

Ghatage commented Jul 12, 2024

Copy link
Copy Markdown
Contributor

@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