You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "Reidddddd (via GitHub)" <gi...@apache.org> on 2023/10/03 08:03:48 UTC
[PR] [Bug] LedgerManagerFactory Info storing in LAYOUT znode is not correct [bookkeeper]
Reidddddd opened a new pull request, #4095:
URL: https://github.com/apache/bookkeeper/pull/4095
Descriptions of the changes in this PR:
### Motivation
Fix a bug described in #<4094>
### Changes
When executing `format` method, as `layoutManager` is inited already as user configured, the `LedgerManagerFactory` info should be read from znode, instead of configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] LedgerManagerFactory Info storing in LAYOUT znode is not correct [bookkeeper]
Posted by "Reidddddd (via GitHub)" <gi...@apache.org>.
Reidddddd commented on code in PR #4095:
URL: https://github.com/apache/bookkeeper/pull/4095#discussion_r1343675520
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManagerFactory.java:
##########
@@ -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();
Review Comment:
as here it wipes the LAYOUT in znode, `factoryClass` by default is null, which will use `HierarchicalLedgerManager` as default in the following `createNewLMFactory`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] LedgerManagerFactory Info storing in LAYOUT znode is not correct [bookkeeper]
Posted by "Reidddddd (via GitHub)" <gi...@apache.org>.
Reidddddd commented on code in PR #4095:
URL: https://github.com/apache/bookkeeper/pull/4095#discussion_r1533465918
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManagerFactory.java:
##########
@@ -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();
Review 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
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] LedgerManagerFactory Info storing in LAYOUT znode is not correct [bookkeeper]
Posted by "Reidddddd (via GitHub)" <gi...@apache.org>.
Reidddddd commented on code in PR #4095:
URL: https://github.com/apache/bookkeeper/pull/4095#discussion_r1343675520
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManagerFactory.java:
##########
@@ -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();
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] LedgerManagerFactory Info storing in LAYOUT znode is not correct [bookkeeper]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #4095:
URL: https://github.com/apache/bookkeeper/pull/4095#discussion_r1372732024
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManagerFactory.java:
##########
@@ -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();
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] LedgerManagerFactory Info storing in LAYOUT znode is not correct [bookkeeper]
Posted by "Reidddddd (via GitHub)" <gi...@apache.org>.
Reidddddd commented on code in PR #4095:
URL: https://github.com/apache/bookkeeper/pull/4095#discussion_r1343682855
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManagerFactory.java:
##########
@@ -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();
Review Comment:
that's why we need to read `ledgerManagerFactory class` info out of `layoutManager` before wiping, and use it to format
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] LedgerManagerFactory Info storing in LAYOUT znode is not correct [bookkeeper]
Posted by "Reidddddd (via GitHub)" <gi...@apache.org>.
Reidddddd commented on code in PR #4095:
URL: https://github.com/apache/bookkeeper/pull/4095#discussion_r1372751149
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManagerFactory.java:
##########
@@ -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();
Review 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
emm, don't get your point, seems to be some misunderstanding here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org