acelyc111 commented on code in PR #1914:
URL:
https://github.com/apache/incubator-pegasus/pull/1914#discussion_r1498553469
##########
src/zookeeper/zookeeper_session.cpp:
##########
@@ -158,27 +167,48 @@ int zookeeper_session::attach(void *callback_owner, const
state_callback &cb)
{
utils::auto_write_lock l(_watcher_lock);
if (nullptr == _handle) {
- if (FLAGS_enable_zookeeper_kerberos) {
+ if (utils::is_empty(FLAGS_sasl_mechanisms_type)) {
Review Comment:
It's better to keep compatibility to retain FLAGS_enable_zookeeper_kerberos.
Since FLAGS_sasl_mechanisms_type is newly introduced, you can validate the
config relationship by DSN_DEFINE_group_validator().
##########
src/zookeeper/zookeeper_session.cpp:
##########
@@ -24,35 +24,44 @@
* THE SOFTWARE.
*/
+#include <sasl/sasl.h>
#include <stdlib.h>
#include <zookeeper/zookeeper.h>
#include <algorithm>
#include <utility>
#include "runtime/app_model.h"
#include "runtime/rpc/rpc_address.h"
+#include "utils/filesystem.h"
#include "utils/flags.h"
#include "utils/fmt_logging.h"
+#include "utils/strings.h"
#include "zookeeper/proto.h"
#include "zookeeper/zookeeper.jute.h"
#include "zookeeper_session.h"
-DSN_DECLARE_bool(enable_zookeeper_kerberos);
-DSN_DEFINE_string(security,
- zookeeper_kerberos_service_name,
- "zookeeper",
- "zookeeper kerberos service name");
-DSN_DEFINE_string(security,
- zookeeper_sasl_service_fqdn,
- "",
- "The FQDN of a Zookeeper server, used in Kerberos
Principal");
// TODO(yingchun): to keep compatibility, the global name is FLAGS_timeout_ms.
The name is not very
// suitable, maybe improve the macro to us another global name.
DSN_DEFINE_int32(zookeeper,
timeout_ms,
30000,
"The timeout of accessing ZooKeeper, in milliseconds");
DSN_DEFINE_string(zookeeper, hosts_list, "", "Zookeeper hosts list");
+DSN_DEFINE_string(zookeeper, sasl_service_name, "zookeeper", "");
+DSN_DEFINE_string(zookeeper,
+ sasl_service_fqdn,
Review Comment:
How about keeping the config names to keep compatible? Otherwise uses have
to update their config files when upgrade to new Pegasus version.
##########
src/zookeeper/zookeeper_session.cpp:
##########
@@ -158,27 +167,48 @@ int zookeeper_session::attach(void *callback_owner, const
state_callback &cb)
{
utils::auto_write_lock l(_watcher_lock);
if (nullptr == _handle) {
- if (FLAGS_enable_zookeeper_kerberos) {
+ if (utils::is_empty(FLAGS_sasl_mechanisms_type)) {
+ _handle = zookeeper_init(
+ FLAGS_hosts_list, global_watcher, FLAGS_timeout_ms, nullptr,
this, 0);
+ } else {
+ int err = sasl_client_init(nullptr);
Review Comment:
To improve the readability, you can reduce the indents, for example:
```
utils::auto_write_lock l(_watcher_lock);
if (nullptr != _handle) {
return;
}
if (kerberos_is_disabled) {
...
return;
}
int err = sasl_client_init(nullptr);
...
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
