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.

Sign up for GitHub

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

Jump to bottom

fix: add id field to bookie socket address struct #545

Closed
wants to merge 2 commits into from
Closed

fix: add id field to bookie socket address struct #545

wants to merge 2 commits into from

Conversation

nodece
Copy link
Contributor

@nodece nodece commented Dec 29, 2021 •
edited

Signed-off-by: Zixuan Liu nodeces@gmail.com

Fix #523

When get the ledger info by pulsarctl bookkeeper ledger get 0, this command returns the following body:

{
  "0": {
    "storeCtime": false,
    "hasPassword": false,
    "metadataFormatVersion": 3,
    "ensembleSize": 1,
    "writeQuorumSize": 1,
    "ackQuorumSize": 1,
    "length": 0,
    "lastEntryId": -1,
    "ctime": 1640766195984,
    "cToken": 4093019187836386319,
    "state": "OPEN",
    "digestType": "CRC32C",
    "allEnsembles": {
      "0": [
        {
          "port": 0,
          "hostname": ""
        }
      ]
    },
    "currentEnsemble": null,
    "password": "",
    "customMetadata": {
      "application": "cHVsc2Fy",
      "component": "bWFuYWdlZC1sZWRnZXI=",
      "pulsar/managed-ledger": "cHVsc2FyL2NsdXN0ZXItYS8xMjcuMC4wLjE6ODA4MS9wZXJzaXN0ZW50L19fY2hhbmdlX2V2ZW50cw=="
    }
  }
}

The allEnsembles.0.id and allEnsembles.0.hostname is nil value. In face, it should be non-nil value.

I checked the bk repo and found the apache/bookkeeper#2404 changes this behavior, so add the id field with omitempty to bookie socket address struct for compatibility.

@nodece nodece requested review from zymap and a team as code owners December 29, 2021 08:49
@nodece nodece mentioned this pull request Dec 29, 2021
Open
@Shawyeok
Copy link

It respects the latest bookkeeper rest api, looks good to me.

zymap
zymap reviewed Dec 30, 2021
View reviewed changes
Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

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

Overall looks good to me.

Could you please also update the CI?

pulsarctl/test/docker/docker-compose.yml

Line 5 in d38b1d8

image: "apache/bookkeeper:4.10.0"

I think we should use the latest image to test this.

@nodece
Copy link
Contributor Author

nodece commented Jan 4, 2022

@zymap Update the bk image will cause the test to fail, I will make a PR after this PR is merged into master.

@zymap
Copy link
Member

zymap commented Jan 5, 2022

But we need a test to cover this, no?

@nodece
Copy link
Contributor Author

nodece commented Jan 5, 2022

@zymap I can add a test to check the id field or host and port field exists.

@zymap
Copy link
Member

zymap commented Jan 5, 2022

The tests which failed mean there have other commands that can't run with the latest bookkeeper, we need to update the bookkeeper to the latest to make sure other commands can run well with the latest bookkeeper. You can do this in another PR as well.

@nodece nodece force-pushed the fix_get_ledger branch 14 times, most recently from d89dd2e to c49d89d Compare January 8, 2022 15:10
@nodece nodece requested a review from zymap January 8, 2022 16:12
@nodece
Copy link
Contributor Author

nodece commented Jan 8, 2022

@zymap Please take a look.

zymap
zymap previously approved these changes Dec 12, 2022
View reviewed changes
@nodece
fix: add id field to bookie socket address struct
bf893ce 
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece
test: fix assert auditor
0be010a 
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece requested a review from zymap December 12, 2022 07:36
@nodece nodece closed this Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zymap zymap Awaiting requested review from zymap zymap is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bk ledger get <ledgerId> output ensemble with empty hostname and port
3 participants
@nodece @Shawyeok @zymap