You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2020/08/20 18:18:48 UTC
[GitHub] [samza] MabelYC opened a new pull request #1420: SAMZA-2574 : improve flexibility of SystemFactory interface
MabelYC opened a new pull request #1420:
URL: https://github.com/apache/samza/pull/1420
This is second pr to supplement commit: 4d97a8d93c47d629072648ac98dabd6a40746321
Changes: Added addition label info to SystemAdmin to help user provide more information to SystemFactory.
API Changes: Added several new functions to help provide more parameter. Won't impact current users.
Upgrade Instructions: None
Usage Instructions: None
----------------------------------------------------------------
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
[GitHub] [samza] sborya commented on a change in pull request #1420: SAMZA-2574 : improve flexibility of SystemFactory interface
Posted by 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
[GitHub] [samza] MabelYC closed pull request #1420: SAMZA-2574 : improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
MabelYC closed pull request #1420:
URL: https://github.com/apache/samza/pull/1420
----------------------------------------------------------------
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
[GitHub] [samza] sborya commented on a change in pull request #1420: SAMZA-2574 : improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
sborya commented on a change in pull request #1420:
URL: https://github.com/apache/samza/pull/1420#discussion_r483283574
##########
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) {
Review comment:
can we change implementation of getSystemAdmins() to {
return getSystemAdmins("").
}
----------------------------------------------------------------
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
[GitHub] [samza] MabelYC commented on a change in pull request #1420: SAMZA-2574 : improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
MabelYC commented on a change in pull request #1420:
URL: https://github.com/apache/samza/pull/1420#discussion_r485226318
##########
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) {
Review comment:
This file is updated. The version you reviewed is the outdated one. Please see: https://github.com/apache/samza/pull/1420/files#diff-2063da999ce81031b6015c31991f60c4R100
----------------------------------------------------------------
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
[GitHub] [samza] sborya commented on a change in pull request #1420: SAMZA-2574 : improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
sborya commented on a change in pull request #1420:
URL: https://github.com/apache/samza/pull/1420#discussion_r485972765
##########
File path: samza-core/src/main/java/org/apache/samza/config/SystemConfig.java
##########
@@ -89,11 +98,14 @@ public SystemConfig(Config config) {
* @return map of system name to {@link SystemAdmin}
*/
public Map<String, SystemAdmin> getSystemAdmins() {
Review comment:
I wonder if it is better to pass the admin label here in getSystemAdmins()?
----------------------------------------------------------------
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