You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by gi...@git.apache.org on 2017/08/31 13:22:14 UTC

[GitHub] nkurihar commented on a change in pull request #721: Parse authParamsString on plugin side

nkurihar commented on a change in pull request #721: Parse authParamsString on plugin side
URL: https://github.com/apache/incubator-pulsar/pull/721#discussion_r136332687
 
 

 ##########
 File path: pulsar-client/src/main/java/org/apache/pulsar/client/api/AuthenticationFactory.java
 ##########
 @@ -41,17 +41,18 @@
      */
     public static final Authentication create(String authPluginClassName, String authParamsString)
             throws UnsupportedAuthenticationException {
-        Map<String, String> authParams = new HashMap<String, String>();
-        if (isNotBlank(authParamsString)) {
-            String[] params = authParamsString.split(",");
-            for (String p : params) {
-                String[] kv = p.split(":");
-                if (kv.length == 2) {
-                    authParams.put(kv[0], kv[1]);
-                }
+        try {
+            if (isNotBlank(authPluginClassName)) {
+                Class<?> authClass = Class.forName(authPluginClassName);
+                Authentication auth = (Authentication) authClass.newInstance();
+                auth.configure(authParamsString);
 
 Review comment:
   I think rdhabalia's suggestion is good enough since what we want to do here is to allow String containing ":" or "," (such as URL) as a parameter.
   
   Introducing new interface sounds nice, however, I think we should do that when we really need plugin-specific parsing.
 
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services