indexDir compatible fix for issue #3430 by StevenLuMT · Pull Request #3433 · apache/bookkeeper

StevenLuMT · 2022-07-28T16:49:06Z

Descriptions of the changes in this PR:

Motivation

Fixes #3430, make cookie validation is compatible with old version.
I think we should check indexDirs when configuration‘s indexDirectories is setted rather than avoiding the check

Changes

add compatible check
add testcase to cover the new code

StevenLuMT · 2022-07-28T17:04:42Z

@horizonzy thanks for your issue,
have a look, does this solve your problem?

bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CookieIndexDirTest.java

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Cookie.java

horizonzy

LGTM

eolivelli

Lgtm

Descriptions of the changes in this PR: ### Motivation Fixes apache#3430, make cookie validation is compatible with old version. I think we should check indexDirs when configuration‘s indexDirectories is setted rather than avoiding the check ### Changes 1. add compatible check 2. add testcase to cover the new code (cherry picked from commit f06800c)

fvigotti · 2023-09-21T09:47:28Z

I'm not sure if I am missing something because this is a year old merged bug .. but updating a cluster from 4.15.4 to 4.16.3 (both using the dockerhub official images )
I still get this incompatibility issue , shouldn't the cookie be compatible and get updated once the 4.16+ bookie starts ?

2023-09-21T09:36:00,280 - ERROR - [main:Main@218] - Failed to build bookie server
org.apache.bookkeeper.bookie.BookieException$InvalidCookieException: Cookie [5
bookieHost: "bk1"
journalDir: "/data/bookkeeper/journal"
ledgerDirs: "1\t/data/bookkeeper/ledgers"
instanceId: "e44f080b-a6ba-4b5e-97f0-0a0303bfd295"
indexDirs: "1\t/data/bookkeeper/index"
] is not matching with [5
bookieHost: "bk1"
journalDir: "/data/bookkeeper/journal"
ledgerDirs: "1\t/data/bookkeeper/ledgers"
instanceId: "e44f080b-a6ba-4b5e-97f0-0a0303bfd295"

a fast check on the code and the problem maybe was because I've used a specified indexDirs instead of the default (ledgerdir) one ? but the code seems to assume that if the cookie is blank then it's the ledgerDirs ( and shouldn't assume that because can be configured differently in the configuration )

https://github.com/StevenLuMT/bookkeeper/blob/905d317c5ab6e700aa3195259c274b55a70540c5/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Cookie.java#L142-L143

as note for others in the same situation my workaround has been to do a rolling update with the reset cookie bin/bookkeeper shell bookieformat -deleteCookie on each bookie

horizonzy · 2023-09-22T03:43:00Z

I'm not sure if I am missing something because this is a year old merged bug .. but updating a cluster from 4.15.4 to 4.16.3 (both using the dockerhub official images ) I still get this incompatibility issue , shouldn't the cookie be compatible and get updated once the 4.16+ bookie starts ?
2023-09-21T09:36:00,280 - ERROR - [main:Main@218] - Failed to build bookie server
org.apache.bookkeeper.bookie.BookieException$InvalidCookieException: Cookie [5
bookieHost: "bk1"
journalDir: "/data/bookkeeper/journal"
ledgerDirs: "1\t/data/bookkeeper/ledgers"
instanceId: "e44f080b-a6ba-4b5e-97f0-0a0303bfd295"
indexDirs: "1\t/data/bookkeeper/index"
] is not matching with [5
bookieHost: "bk1"
journalDir: "/data/bookkeeper/journal"
ledgerDirs: "1\t/data/bookkeeper/ledgers"
instanceId: "e44f080b-a6ba-4b5e-97f0-0a0303bfd295"
a fast check on the code and the problem maybe was because I've used a specified indexDirs instead of the default (ledgerdir) one ? but the code seems to assume that if the cookie is blank then it's the ledgerDirs ( and shouldn't assume that because can be configured differently in the configuration )

https://github.com/StevenLuMT/bookkeeper/blob/905d317c5ab6e700aa3195259c274b55a70540c5/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Cookie.java#L142-L143

as note for others in the same situation my workaround has been to do a rolling update with the reset cookie bin/bookkeeper shell bookieformat -deleteCookie on each bookie

Ok, let me check it.

horizonzy · 2023-09-22T05:59:46Z

I'm not sure if I am missing something because this is a year old merged bug .. but updating a cluster from 4.15.4 to 4.16.3 (both using the dockerhub official images ) I still get this incompatibility issue , shouldn't the cookie be compatible and get updated once the 4.16+ bookie starts ?
2023-09-21T09:36:00,280 - ERROR - [main:Main@218] - Failed to build bookie server
org.apache.bookkeeper.bookie.BookieException$InvalidCookieException: Cookie [5
bookieHost: "bk1"
journalDir: "/data/bookkeeper/journal"
ledgerDirs: "1\t/data/bookkeeper/ledgers"
instanceId: "e44f080b-a6ba-4b5e-97f0-0a0303bfd295"
indexDirs: "1\t/data/bookkeeper/index"
] is not matching with [5
bookieHost: "bk1"
journalDir: "/data/bookkeeper/journal"
ledgerDirs: "1\t/data/bookkeeper/ledgers"
instanceId: "e44f080b-a6ba-4b5e-97f0-0a0303bfd295"
a fast check on the code and the problem maybe was because I've used a specified indexDirs instead of the default (ledgerdir) one ? but the code seems to assume that if the cookie is blank then it's the ledgerDirs ( and shouldn't assume that because can be configured differently in the configuration )

https://github.com/StevenLuMT/bookkeeper/blob/905d317c5ab6e700aa3195259c274b55a70540c5/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Cookie.java#L142-L143

as note for others in the same situation my workaround has been to do a rolling update with the reset cookie bin/bookkeeper shell bookieformat -deleteCookie on each bookie

Yes, it's indeed a problem. If the cookie doesn't has indexDirs, we shouldn't consider ledgerDirs as its value.

horizonzy · 2023-09-22T08:43:17Z

I'm not sure if I am missing something because this is a year old merged bug .. but updating a cluster from 4.15.4 to 4.16.3 (both using the dockerhub official images ) I still get this incompatibility issue , shouldn't the cookie be compatible and get updated once the 4.16+ bookie starts ?
2023-09-21T09:36:00,280 - ERROR - [main:Main@218] - Failed to build bookie server
org.apache.bookkeeper.bookie.BookieException$InvalidCookieException: Cookie [5
bookieHost: "bk1"
journalDir: "/data/bookkeeper/journal"
ledgerDirs: "1\t/data/bookkeeper/ledgers"
instanceId: "e44f080b-a6ba-4b5e-97f0-0a0303bfd295"
indexDirs: "1\t/data/bookkeeper/index"
] is not matching with [5
bookieHost: "bk1"
journalDir: "/data/bookkeeper/journal"
ledgerDirs: "1\t/data/bookkeeper/ledgers"
instanceId: "e44f080b-a6ba-4b5e-97f0-0a0303bfd295"
a fast check on the code and the problem maybe was because I've used a specified indexDirs instead of the default (ledgerdir) one ? but the code seems to assume that if the cookie is blank then it's the ledgerDirs ( and shouldn't assume that because can be configured differently in the configuration )

https://github.com/StevenLuMT/bookkeeper/blob/905d317c5ab6e700aa3195259c274b55a70540c5/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Cookie.java#L142-L143

as note for others in the same situation my workaround has been to do a rolling update with the reset cookie bin/bookkeeper shell bookieformat -deleteCookie on each bookie

I think we should upgrade the cookie version, or we can't know the indexDirs is a really null or just a old version without indexDirs.

horizonzy · 2023-09-26T01:46:37Z

I'm not sure if I am missing something because this is a year old merged bug .. but updating a cluster from 4.15.4 to 4.16.3 (both using the dockerhub official images ) I still get this incompatibility issue , shouldn't the cookie be compatible and get updated once the 4.16+ bookie starts ?
2023-09-21T09:36:00,280 - ERROR - [main:Main@218] - Failed to build bookie server
org.apache.bookkeeper.bookie.BookieException$InvalidCookieException: Cookie [5
bookieHost: "bk1"
journalDir: "/data/bookkeeper/journal"
ledgerDirs: "1\t/data/bookkeeper/ledgers"
instanceId: "e44f080b-a6ba-4b5e-97f0-0a0303bfd295"
indexDirs: "1\t/data/bookkeeper/index"
] is not matching with [5
bookieHost: "bk1"
journalDir: "/data/bookkeeper/journal"
ledgerDirs: "1\t/data/bookkeeper/ledgers"
instanceId: "e44f080b-a6ba-4b5e-97f0-0a0303bfd295"
a fast check on the code and the problem maybe was because I've used a specified indexDirs instead of the default (ledgerdir) one ? but the code seems to assume that if the cookie is blank then it's the ledgerDirs ( and shouldn't assume that because can be configured differently in the configuration )

https://github.com/StevenLuMT/bookkeeper/blob/905d317c5ab6e700aa3195259c274b55a70540c5/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Cookie.java#L142-L143

as note for others in the same situation my workaround has been to do a rolling update with the reset cookie bin/bookkeeper shell bookieformat -deleteCookie on each bookie

I have submitted a PR to fix it. Could you help to check it? #4087

Descriptions of the changes in this PR: ### Motivation Fixes apache#3430, make cookie validation is compatible with old version. I think we should check indexDirs when configuration‘s indexDirectories is setted rather than avoiding the check ### Changes 1. add compatible check 2. add testcase to cover the new code

StevenLuMT mentioned this pull request Jul 29, 2022

Fix cookie compatible problem. #3431

Closed

horizonzy reviewed Jul 29, 2022

View reviewed changes

bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CookieIndexDirTest.java Outdated Show resolved Hide resolved

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Cookie.java Outdated Show resolved Hide resolved

hangc0276 requested review from dlg99, eolivelli, hangc0276, hezhangjian, nicoloboschi and zymap July 29, 2022 07:31

hangc0276 assigned StevenLuMT Jul 29, 2022

hangc0276 added this to the 4.16.0 milestone Jul 29, 2022

StevenLuMT force-pushed the master_indexDirCompatibleFix branch from 1a88ef0 to a7e3c28 Compare July 29, 2022 07:39

indexDir compatible fix for issue apache#3430

905d317

StevenLuMT force-pushed the master_indexDirCompatibleFix branch from a7e3c28 to 905d317 Compare July 29, 2022 07:55

horizonzy approved these changes Jul 29, 2022

View reviewed changes

StevenLuMT mentioned this pull request Jul 29, 2022

Improve the function: set dir name to store index files. #3419

Closed

11 tasks

hezhangjian approved these changes Jul 30, 2022

View reviewed changes

eolivelli approved these changes Jul 30, 2022

View reviewed changes

hangc0276 approved these changes Jul 31, 2022

View reviewed changes

hangc0276 merged commit f06800c into apache:master Jul 31, 2022

hangc0276 added type/bug area/bookie labels Jul 31, 2022

horizonzy mentioned this pull request Sep 25, 2023

Make after version5 cookie index dir compatible with before version5. #4087

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

indexDir compatible fix for issue #3430#3433

indexDir compatible fix for issue #3430#3433
hangc0276 merged 1 commit intoapache:masterfrom
StevenLuMT:master_indexDirCompatibleFix

StevenLuMT commented Jul 28, 2022 •

edited

Loading

Uh oh!

StevenLuMT commented Jul 28, 2022

Uh oh!

Uh oh!

Uh oh!

horizonzy left a comment

Uh oh!

eolivelli left a comment

Uh oh!

fvigotti commented Sep 21, 2023 •

edited

Loading

Uh oh!

horizonzy commented Sep 22, 2023

Uh oh!

horizonzy commented Sep 22, 2023

Uh oh!

horizonzy commented Sep 22, 2023

Uh oh!

horizonzy commented Sep 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

StevenLuMT commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Uh oh!

StevenLuMT commented Jul 28, 2022

Uh oh!

Uh oh!

Uh oh!

horizonzy left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

fvigotti commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

horizonzy commented Sep 22, 2023

Uh oh!

horizonzy commented Sep 22, 2023

Uh oh!

horizonzy commented Sep 22, 2023

Uh oh!

horizonzy commented Sep 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

StevenLuMT commented Jul 28, 2022 •

edited

Loading

fvigotti commented Sep 21, 2023 •

edited

Loading