[GitHub] [samza] sborya commented on a change in pull request #1420: SAMZA-2574 : improve flexibility of SystemFactory interface

[GitBox <gi...@apache.org>]

sborya commented on a change in pull request #1420:
URL: https://github.com/apache/samza/pull/1420#discussion_r477522486



##########
File path: samza-core/src/main/java/org/apache/samza/config/SystemConfig.java
##########
@@ -106,6 +123,19 @@ public SystemAdmin getSystemAdmin(String systemName) {
     return getSystemAdmins().get(systemName);
   }
 
+  /**
+   * Get {@link SystemAdmin} instance for given system name with an adminLabel.
+   * This function provides an extra input parameter to {@link #getSystemAdmin}, which can be used to provide extra
+   * information for the admin instance, e.g. ownership of client instance, to help better identify admins in logs,
+   * threads and client instances etc., along with other relevant information like systemName.
+   *
+   * @param systemName System name
+   * @return SystemAdmin of the system if it exists, otherwise null.
+   */
+  public SystemAdmin getSystemAdmin(String systemName, String adminLabel) {
+    return getSystemAdmins(adminLabel).get(systemName);

Review comment:
       I understand this is an existing code, but it seems weird that we create ALL admins EVERY time just to get a single one.
   Either we should create a single Admin, or we can cache the results of the previous getAdmins().

##########
File path: samza-core/src/main/java/org/apache/samza/config/SystemConfig.java
##########
@@ -96,6 +96,23 @@ public SystemConfig(Config config) {
               .getAdmin(systemNameToFactoryEntry.getKey(), this)));
   }
 
+  /**
+   * Get {@link SystemAdmin} instances for all the systems defined in this config and set with an adminLabel.
+   * This function provides an extra input parameter to {@link #getSystemAdmins}, which can be used to provide extra
+   * information for the admin instance, e.g. ownership of client instance, to help better identify admins in logs,
+   * threads and client instances etc., along with other relevant information like systemName.
+   *
+   * @param adminLabel a string to provide info the admin instance.
+   * @return map of system name to {@link SystemAdmin}
+   */
+  public Map<String, SystemAdmin> getSystemAdmins(String adminLabel) {
+    return getSystemFactories().entrySet()
+        .stream()
+        .collect(Collectors.toMap(Entry::getKey,
+          systemNameToFactoryEntry -> systemNameToFactoryEntry.getValue()
+              .getAdmin(systemNameToFactoryEntry.getKey(), this, adminLabel)));

Review comment:
       can you also change the other getSystemAdmins() to use this new one with the label = ""




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org