[Bug] LedgerManagerFactory Info storing in LAYOUT znode is not correct #4095
Conversation
| 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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
that's why we need to read ledgerManagerFactory class info out of layoutManager before wiping, and use it to format
There was a problem hiding this comment.
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
There was a problem hiding this comment.
The LAYOUT is
HierarchicalLedgerManagerbut the user configuration isLongHierarchicalLedgerManagerFactory, I think we should fail the format. Otherwise, the user may think the LAYOUT has been formatted toLongHierarchicalLedgerManagerFactory, but actually it doesn't
emm, don't get your point, seems to be some misunderstanding here
There was a problem hiding this comment.
The LAYOUT is
HierarchicalLedgerManagerbut the user configuration isLongHierarchicalLedgerManagerFactory, I think we should fail the format. Otherwise, the user may think the LAYOUT has been formatted toLongHierarchicalLedgerManagerFactory, 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
| 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(); |
There was a problem hiding this comment.
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
Descriptions of the changes in this PR:
Motivation
Fix a bug described in issue: #4094
Changes
When executing
formatmethod, aslayoutManageris inited already as user configured, theLedgerManagerFactoryinfo should be read from znode, instead of configuration.