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

[Bug] LedgerManagerFactory Info storing in LAYOUT znode is not correct #4095

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[Bug] LedgerManagerFactory Info storing in LAYOUT znode is not correct #4095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0250058
[Bug] LedgerManagerFactory Info storing in LAYOUT znode is not correct
Reidddddd Oct 3, 2023
8ceef4a
remote unrelated changes
Reidddddd Oct 3, 2023
4611edc
checkstyle fix ii
Reidddddd Oct 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions ...eeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManagerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,11 @@ public void format(AbstractConfiguration<?> conf, LayoutManager layoutManager)

Class<? extends LedgerManagerFactory> factoryClass;
try {
factoryClass = conf.getLedgerManagerFactoryClass();
} catch (ConfigurationException e) {
throw new IOException("Failed to get ledger manager factory class from configuration : ", e);
factoryClass = (Class<? extends LedgerManagerFactory>)
Class.forName(layoutManager.readLedgerLayout().getManagerFactoryClass());
} catch (ClassNotFoundException e) {
// should not reach here, as LayoutManager was successfully inited
throw new IOException("Failed to read ledger manager factory class from LAYOUT : ", e);
}

layoutManager.deleteLedgerLayout();
Copy link
Contributor Author

@Reidddddd Reidddddd Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

as here it wipes the LAYOUT in znode, factoryClass by default is null, which will use HierarchicalLedgerManager as default in the following createNewLMFactory. So if user configure is not the default HierarchicalLedgerManager, this bug will occur

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's why we need to read ledgerManagerFactory class info out of layoutManager before wiping, and use it to format

Copy link
Contributor

Choose a reason for hiding this comment

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

The LAYOUT is HierarchicalLedgerManager but the user configuration is LongHierarchicalLedgerManagerFactory, I think we should fail the format. Otherwise, the user may think the LAYOUT has been formatted to LongHierarchicalLedgerManagerFactory, but actually it doesn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The LAYOUT is HierarchicalLedgerManager but the user configuration is LongHierarchicalLedgerManagerFactory, I think we should fail the format. Otherwise, the user may think the LAYOUT has been formatted to LongHierarchicalLedgerManagerFactory, but actually it doesn't

emm, don't get your point, seems to be some misunderstanding here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The LAYOUT is HierarchicalLedgerManager but the user configuration is LongHierarchicalLedgerManagerFactory, I think we should fail the format. Otherwise, the user may think the LAYOUT has been formatted to LongHierarchicalLedgerManagerFactory, but actually it doesn't

Let's put it straight, user configured longhierarchical and executed format which means using `LongHierarchicalLedgerManagerFactory.

But the fact, znode is written HierarchicalLedgerManager which is not configured, it is just a value from if null then by default

So in the UT, if w/o this fix, the second time format will fail because of mismatching HierarchicalLedgerManager and LongHierarchicalLedgerManagerFactory. Normally, it should be good to execute format as much as many time as configurations are the same

Expand Down
15 changes: 15 additions & 0 deletions bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1770,4 +1770,19 @@ public void testBookieIdChange() throws Exception {
fail();
}
}

@Test
public void testFormatTwiceWithNonDefaultHierarchicalType() throws Exception {
File tmpDir = tmpDirs.createNew("bookie", "test");
final String zkRoot = "/ledgers3";

final ServerConfiguration conf = TestBKConfiguration.newServerConfiguration()
.setJournalDirName(tmpDir.getPath())
.setLedgerDirNames(new String[] { tmpDir.getPath() })
.setMetadataServiceUri(zkUtil.getMetadataServiceUri(zkRoot, "longhierarchical"))
.setZkTimeout(5000);
BookKeeperAdmin.format(conf, false, false);
BookKeeperAdmin.format(conf, false, true);
}

}