michaeljmarshall commented on code in PR #15462:
URL: https://github.com/apache/pulsar/pull/15462#discussion_r866466456
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/BookKeeperClientFactoryImpl.java:
##########
@@ -159,6 +159,7 @@ ClientConfiguration
createBkClientConfiguration(MetadataStoreExtended store, Ser
bkConf.setProperty(bkExtraConfigKey, value);
}
});
+
bkConf.setWaitTimeoutOnBackpressureMillis(conf.getWaitTimeoutOnBackpressureMillis());
Review Comment:
Setting this configuration here would override an existing
`bookkeeper_waitTimeoutOnBackpressureMillis` configuration in the `broker.conf`
file. That could lead a user to unexpectedly get the Pulsar default value. I
think we should move this line to line 152.
##########
conf/broker.conf:
##########
@@ -930,6 +930,22 @@ bookkeeperExplicitLacIntervalInMills=0
# Expose bookkeeper client managed ledger stats to prometheus. default is false
# bookkeeperClientExposeStatsToPrometheus=false
+# Timeout controlling wait on request send in case of unresponsive bookie(s).
+# Timeout value:
+# - negative value disables the feature
+# - positive to allow wait up to specified number of milliseconds
+# - 0 to allow request to fail immediately.
+# Default is -1 (disabled)
+#########
+# # Should be used in combination with the bookkeeper-side
+# # backpressure configuration, such as
+# # maxAddsInProgressLimit = (positive value)
+# # maxReadsInProgressLimit = (positive value)
+# # closeChannelOnResponseTimeout = true
+# # waitTimeoutOnResponseBackpressureMs = (positive value)
+#########
+# waitTimeoutOnBackpressureMillis = -1
Review Comment:
Nit: it looks like there is a convention to prefix bookkeeper configs with
`bookkeeper` in this section. I don't feel too strong on this, but I think it'd
be more consistent to use either `bookkeeperWaitTimeoutOnBackpressureMillis` or
`bookkeeperClientWaitTimeoutOnBackpressureMillis`.
Another nit: I see that this defaults as a commented out config. I think it
might be helpful to make this a first class member of this file so that users
don't need to add the `PULSAR_PREFIX_` to the env var.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
