You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2021/09/08 04:26:49 UTC
[GitHub] [bookkeeper] gaozhangmin opened a new pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
gaozhangmin opened a new pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788
`BookieAddressResolver` should be set before `((Configurable) dnsResolver).setConf(conf);`
It will throw npe. when pulsar `ZkBookieRackAffinityMapping` invoke getBookieAddressResolver`
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-922948158
We will merge as soon as CI passes.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704897937
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,12 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
Review comment:
Agree
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704078096
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,14 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
} else {
String dnsResolverName = conf.getString(REPP_DNS_RESOLVER_CLASS, ScriptBasedMapping.class.getName());
try {
dnsResolver = ReflectionUtils.newInstance(dnsResolverName, DNSToSwitchMapping.class);
+ if (dnsResolver != null) {
Review comment:
good catch @lhotari !
it is always non null 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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704977646
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,12 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
Review comment:
Configure dnsResolver where it is created/before passing it to downstream components.
This may be a better way fixing the issue. But I am not sure it will be accepted, cause too many code changes,
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704077734
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,14 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
} else {
String dnsResolverName = conf.getString(REPP_DNS_RESOLVER_CLASS, ScriptBasedMapping.class.getName());
try {
dnsResolver = ReflectionUtils.newInstance(dnsResolverName, DNSToSwitchMapping.class);
+ if (dnsResolver != null) {
Review comment:
is there some way how dnsResolver could be null 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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin removed a comment on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin removed a comment on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-926313820
rerun failure tests
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-915942478
> I believe we are on our way.
>
> Is it possible to add some test cases, or enhance some existing test cases, in order to cover the changes you made ?
I think the unit test `TestRackawareEnsemblePlacementPolicy` already covered this change.
like `repp.initialize(conf, Optional.<DNSToSwitchMapping>empty(), timer,
DISABLE_ALL, NullStatsLogger.INSTANCE, BookieSocketAddress.LEGACY_BOOKIEID_RESOLVER);`
the dnsResolver is null, will reach the condition `!optionalDnsResolver.isPresent() `
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-928730435
@eolivelli the failed tests come from timeouts, please rerun failed tests
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-928730435
@eolivelli the failed tests come from timeouts, please rerun failed tests
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-922945594
any problem with this pr?
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-942232656
@eolivelli
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lhotari commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-915036171
Thanks for providing the log message @gaozhangmin . It looks like the logging is broken since the provided sample log message didn't include the stacktrace. I'd propose to fix it in the same PR since it's closely related to the change that is being made.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin removed a comment on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin removed a comment on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-927704662
rerun failure tests
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704971755
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,12 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
Review comment:
if dnsResolver is null, `RackawareEnsemblePlacementPolicyImpl.java ` will create a new configurable resolver. here, bookieAddressResolver should be set. or ,it will throw NPE
I checked the code, `ZoneawareEnsemblePlacementPolicyImpl` also has the same problem, i will fix it.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704971755
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,12 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
Review comment:
if dnsResolver is null, `RackawareEnsemblePlacementPolicyImpl.java ` will create a Configurable resolver. here, bookieAddressResolver should be set. or ,it will throw NPE
I checked the code, `ZoneawareEnsemblePlacementPolicyImpl` also has the same problem, i will fix it.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-914902644
@hangc0276 PTAL
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r705928705
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,12 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
Review comment:
Remove duplicate setBookieAddressResolver when optionalDnsResolver isPresent
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap merged pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
zymap merged pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-915805174
Sorry I meant:
**I would NOT prefer** to refactor interfaces or pluggable components in order to not introduce API changes, in that case it would be harder to upgrade BK
So this change in this current form works for me.
hat said, adding a simple test case that reproduces the NPE will be better, otherwise we could break ZkBookieRackAffinityMapping again
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lhotari commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-915015882
> there no stacktrace in log , only java.lang.nullpointexception
The log line with that could also help locate this issue when someone with the same problem searches for it. Please share.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-927571173
@dlg99 Please help approve runn workflows
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-927700982
run failed checks
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704897937
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,12 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
Review comment:
Agree
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704577640
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,12 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
Review comment:
I'd assume that provided `optionalDnsResolver` should be ready to use / have the `bookieAddressResolver` set, as it happens there:
https://github.com/apache/bookkeeper/blob/97818f5123999396e66f5246420d3c7e3d25f53d/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java#L500-L507
Have you traced why we happen to have the optional dns resolver provided here yet not properly configured?
I think the right place to fix the issue is where the dns resolver is created and passed to the dnsResolver.
Shall we move bookieAddressResolver into DNSToSwitchMapping constructor (or Builder?) and make it mandatory (preconditions check)?
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704939167
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,12 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
Review comment:
@gaozhangmin `RackawareEnsemblePlacementPolicyImpl` gets Optional (stack from `initializeEnsemblePlacementPolicy` down to the policy will do `Optional.ofNullable(dnsResolver)` to pass it.)
`Optional.isPresent()` in `RackawareEnsemblePlacementPolicyImpl` would be false in case of null (which is handled).
Where is the dnsResolver created and set, how did it end up partially configured?
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704087386
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,14 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
} else {
String dnsResolverName = conf.getString(REPP_DNS_RESOLVER_CLASS, ScriptBasedMapping.class.getName());
try {
dnsResolver = ReflectionUtils.newInstance(dnsResolverName, DNSToSwitchMapping.class);
+ if (dnsResolver != null) {
Review comment:
remove null checker
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-915785410
Canbyou add a test cases that shows the problem?
This way it will be clear what we are fixing and that you fixed it.
I would prefer to refactor interfaces or pluggable components in order to not introduce API changes, in that case it would be harder to upgrade BK
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lhotari commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-914981560
@gaozhangmin Can you add the stacktrace containing the NPE to the description? That could help find this PR if someone searches by stacktrace elements?
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-926313820
rerun failure tests
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lhotari edited a comment on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-914981560
@gaozhangmin Can you add the stacktrace containing the NPE to the description? That could help find this PR if someone searches by stacktrace elements.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-914958267
pulsar also has a related issue https://github.com/apache/pulsar/pull/11947
@eolivelli @lhotari @nicoloboschi PTAL
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-915813572
> Sorry I meant:
>
> **I would NOT prefer** to refactor interfaces or pluggable components in order to not introduce API changes, in that case it would be harder to upgrade BK
>
> So this change in this current form works for me.
> That said, adding a simple test case that reproduces the NPE will be better, otherwise we could break ZkBookieRackAffinityMapping again
>
> If you have the error on ZkBookieRackAffinityMapping you could try to use the bookieAddressResolver in the way that ZkBookieRackAffinityMapping does, but in a dummy implementation
Unit test added, PTAL
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-915059630
> Thanks for providing the log message @gaozhangmin . It looks like the logging is broken since the provided sample log message didn't include the stacktrace. I'd propose to fix it in the same PR since it's closely related to the change that is being made.
>
> This looks invalid:
> `LOG.error("Failed to initialize DNS Resolver {}, used default subnet resolver : {} {}", dnsResolverName, re, re.getMessage());`
>
> I wonder why it's not simply
> `LOG.error("Failed to initialize DNS Resolver {}, using default subnet resolver.", dnsResolverName, re);` ?
changed
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-927704662
rerun failure tests
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704100729
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -282,9 +284,6 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
}
}
}
- if (dnsResolver != null) {
- dnsResolver.setBookieAddressResolver(bookieAddressResolver);
Review comment:
This logic is missing when dnsResolver gets set on line 276
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli edited a comment on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
eolivelli edited a comment on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-915805174
Sorry I meant:
**I would NOT prefer** to refactor interfaces or pluggable components in order to not introduce API changes, in that case it would be harder to upgrade BK
So this change in this current form works for me.
That said, adding a simple test case that reproduces the NPE will be better, otherwise we could break ZkBookieRackAffinityMapping again
If you have the error on ZkBookieRackAffinityMapping you could try to use the bookieAddressResolver in the way that ZkBookieRackAffinityMapping does, but in a dummy implementation
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-942232656
@eolivelli
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-915797127
> I would prefer to refactor interfaces or pluggable components in order to not introduce API changes, in that case it would be harder to upgrade BK
This need pulsar `org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping` class, I do not know how to test this.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-926316328
@eolivelli Please approve running workflows
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-923004404
The failed tests seems coming from timeouts. it's not related with this change.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r705502700
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,12 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
Review comment:
@gaozhangmin let me reiterate my point.
dns resolver is either null or not in this constructor https://github.com/apache/bookkeeper/blob/c7236adc3cb659e65ae5ce53b7156569d7f50ebd/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java#L410 and it is used to initialize placement policy there. In case it is null the policy will create and initialize (correctly, with address resolver) a new one. If it is not null, the constructor will set the resolver.
The change below, where the policy creates the new dnsResolver, now sets the address resolver is absolutely fine and a fix I am agreeing with.
What I do not understand is why do set the address resolver in case above:
```
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
// Why is the next line needed? the policy have to have address resolver set already
//
dnsResolver.setBookieAddressResolver(bookieAddressResolver);
```
The BK client constructor should not be used directly (outside of unit tests) and the builder should be used.
For example:
- here the dns resolver is passed to the builder: https://github.com/apache/bookkeeper/blob/c7236adc3cb659e65ae5ce53b7156569d7f50ebd/stream/distributedlog/core/src/main/java/org/apache/distributedlog/BookKeeperClient.java#L110-L125 and I'd expect it to get configured with the address resolver.
- here the dns resolver is not passed to the builder https://github.com/apache/bookkeeper/blob/9f06a4ce82f61a9dc4b086650a301734332f4246/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java#L270 and I expect placement policy to create and iniialize a new one.
So, either the part is I am confused about is not actually needed or I am missing specific scenario, but in that case it needs to be addressed differently.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-915026495
> > there no stacktrace in log , only java.lang.nullpointexception
>
> The log line with that could also help locate this issue when someone with the same problem searches for it. Please share.
added
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704900514
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,12 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
DNSToSwitchMapping dnsResolver;
if (optionalDnsResolver.isPresent()) {
dnsResolver = optionalDnsResolver.get();
+ dnsResolver.setBookieAddressResolver(bookieAddressResolver);
Review comment:
Because here it missed condition that dnsResolver is null, so bookieAddressResolver would not be set
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-914999007
> @gaozhangmin Can you add the stacktrace containing the NPE to the description? That could help find this PR if someone searches by stacktrace elements.
there no stacktrace in log , only java.lang.nullpointexception
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lhotari edited a comment on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-915036171
Thanks for providing the log message @gaozhangmin . It looks like the logging is broken since the provided sample log message didn't include the stacktrace. I'd propose to fix it in the same PR since it's closely related to the change that is being made.
This looks invalid:
`LOG.error("Failed to initialize DNS Resolver {}, used default subnet resolver : {} {}",
dnsResolverName, re, re.getMessage());`
I wonder why it's not simply
`LOG.error("Failed to initialize DNS Resolver {}, used default subnet resolver.",
dnsResolverName, re);` ?
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin removed a comment on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin removed a comment on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-927700982
run failed checks
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-938293608
@eolivelli CI passes. please merge this pr
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lhotari edited a comment on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-915036171
Thanks for providing the log message @gaozhangmin . It looks like the logging is broken since the provided sample log message didn't include the stacktrace. I'd propose to fix it in the same PR since it's closely related to the change that is being made.
This looks invalid:
`LOG.error("Failed to initialize DNS Resolver {}, used default subnet resolver : {} {}",
dnsResolverName, re, re.getMessage());`
I wonder why it's not simply
`LOG.error("Failed to initialize DNS Resolver {}, using default subnet resolver.",
dnsResolverName, re);` ?
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin removed a comment on pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver
Posted by GitBox <gi...@apache.org>.
gaozhangmin removed a comment on pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#issuecomment-927704662
rerun failure tests
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org