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/04 01:37:59 UTC
[GitHub] [samza] MabelYC opened a new pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
MabelYC opened a new pull request #1408:
URL: https://github.com/apache/samza/pull/1408
improve SystemFactory interface by providing callerClass so that we can access more information of the clients created.
API Changes: Added three default functions in SystemFactory. Since they are default functions, they won't make any impact to current users.
Upgrade Instructions: N/A
Usage Instructions: N/A
----------------------------------------------------------------
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] prateekm commented on a change in pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466669710
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
Why assume that it's used as prefix in the API? Maybe just call this consumerName/consumerId and let the actual implementation figure out how to use it?
----------------------------------------------------------------
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 pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
MabelYC commented on pull request #1408:
URL: https://github.com/apache/samza/pull/1408#issuecomment-669501113
> @MabelYC Please also explain (in the description) why this change is required and how this will be used.
Sure.
----------------------------------------------------------------
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] prateekm commented on a change in pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466759501
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
@MabelYC I mean Javadocs. The page you linked is auto-generated from Javadocs.
----------------------------------------------------------------
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] prateekm commented on pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on pull request #1408:
URL: https://github.com/apache/samza/pull/1408#issuecomment-669388897
@cameronlee314 can you take a look?
----------------------------------------------------------------
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 #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
MabelYC commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466726991
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
Not sure if it is better. With deprecated, user will still have to implement them later. Can it work as we expect?
----------------------------------------------------------------
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] prateekm commented on a change in pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466668567
##########
File path: samza-core/src/main/java/org/apache/samza/util/DiagnosticsUtil.java
##########
@@ -144,7 +145,8 @@ public MetadataFileContents(String version, String metricsSnapshot) {
}
SystemFactory systemFactory = ReflectionUtil.getObj(diagnosticsSystemFactoryName.get(), SystemFactory.class);
SystemProducer systemProducer =
- systemFactory.getProducer(diagnosticsSystemStream.getSystem(), config, new MetricsRegistryMap());
+ systemFactory.getProducer(diagnosticsSystemStream.getSystem(), config, new MetricsRegistryMap(),
+ MethodHandles.lookup().lookupClass().getSimpleName());
Review comment:
Why not use Class.getSimpleName() here too?
----------------------------------------------------------------
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 #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
MabelYC commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466730662
##########
File path: samza-core/src/main/java/org/apache/samza/util/DiagnosticsUtil.java
##########
@@ -144,7 +145,8 @@ public MetadataFileContents(String version, String metricsSnapshot) {
}
SystemFactory systemFactory = ReflectionUtil.getObj(diagnosticsSystemFactoryName.get(), SystemFactory.class);
SystemProducer systemProducer =
- systemFactory.getProducer(diagnosticsSystemStream.getSystem(), config, new MetricsRegistryMap());
+ systemFactory.getProducer(diagnosticsSystemStream.getSystem(), config, new MetricsRegistryMap(),
+ MethodHandles.lookup().lookupClass().getSimpleName());
Review comment:
Got it. Will do.
----------------------------------------------------------------
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 #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
MabelYC commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466724607
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
by "add some documentation" you mean add javadoc or open source documentation?
----------------------------------------------------------------
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 #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
MabelYC commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466727134
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
good point. Thanks. Will do.
----------------------------------------------------------------
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] prateekm commented on a change in pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466669710
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
Why assume that it's used as prefix in the API? Maybe just call this consumerId and let the actual implementation figure out how to use it?
----------------------------------------------------------------
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 #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
sborya commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r469417160
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -27,9 +27,56 @@
* a particular system, as well as the accompanying {@link org.apache.samza.system.SystemAdmin}.
*/
public interface SystemFactory {
+ @Deprecated
SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemAdmin getAdmin(String systemName, Config config);
+
+ /**
+ * This function provides an extra input parameter to {@link #getConsumer}, which can be used to provide extra
+ * information for the consumer instance, e.g. ownership of client instance, to helper better identify consumers in logs,
Review comment:
s/helper/help
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -27,9 +27,56 @@
* a particular system, as well as the accompanying {@link org.apache.samza.system.SystemAdmin}.
*/
public interface SystemFactory {
+ @Deprecated
SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemAdmin getAdmin(String systemName, Config config);
+
+ /**
+ * This function provides an extra input parameter to {@link #getConsumer}, which can be used to provide extra
+ * information for the consumer instance, e.g. ownership of client instance, to helper better identify consumers in logs,
+ * threads and client instances etc., along with other relevant information like systemName.
+ *
+ * @param systemName The name of the system to create consumer for.
+ * @param config The config to create consumer with.
+ * @param registry MetricsRegistry to which to publish consumer specific metrics.
+ * @param consumerLabel a string to provide info the consumer instance.
+ * @return A SystemConsumer
+ */
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerLabel) {
+ return getConsumer(systemName, config, registry);
+ }
+
+ /**
+ * This function provides an extra input parameter to {@link #getProducer}, which can be used to provide extra
+ * information for the producer instance, e.g. ownership of client instance, to helper better identify producers in logs,
Review comment:
see above.
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -27,9 +27,56 @@
* a particular system, as well as the accompanying {@link org.apache.samza.system.SystemAdmin}.
*/
public interface SystemFactory {
+ @Deprecated
SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemAdmin getAdmin(String systemName, Config config);
+
+ /**
+ * This function provides an extra input parameter to {@link #getConsumer}, which can be used to provide extra
+ * information for the consumer instance, e.g. ownership of client instance, to helper better identify consumers in logs,
+ * threads and client instances etc., along with other relevant information like systemName.
+ *
+ * @param systemName The name of the system to create consumer for.
+ * @param config The config to create consumer with.
+ * @param registry MetricsRegistry to which to publish consumer specific metrics.
+ * @param consumerLabel a string to provide info the consumer instance.
+ * @return A SystemConsumer
+ */
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerLabel) {
+ return getConsumer(systemName, config, registry);
+ }
+
+ /**
+ * This function provides an extra input parameter to {@link #getProducer}, which can be used to provide extra
+ * information for the producer instance, e.g. ownership of client instance, to helper better identify producers in logs,
+ * threads and client instances etc., along with other relevant information like systemName.
+ *
+ * @param systemName The name of the system to create producer for.
+ * @param config The config to create producer with.
+ * @param registry MetricsRegistry to which to publish producer specific metrics.
+ * @param producerLabel a string to provide info the producer instance.
+ * @return A SystemProducer
+ */
+ default SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry, String producerLabel) {
+ return getProducer(systemName, config, registry);
+ }
+
+ /**
+ * This function provides an extra input parameter to {@link #getAdmin}, which can be used to provide extra
+ * information for the admin instance, e.g. ownership of client instance, to helper better identify admins in logs,
Review comment:
see above.
----------------------------------------------------------------
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 merged pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
sborya merged pull request #1408:
URL: https://github.com/apache/samza/pull/1408
----------------------------------------------------------------
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] prateekm commented on a change in pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466670112
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
Please add method Javadocs clarifying what the parameters are, and how these methods are different than the ones above.
----------------------------------------------------------------
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 #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
sborya commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466696055
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
I think we don't want caller to assume that this is the id and it must be used as is. The idea is that it should be a unique piece of info that can be used to construct an id. Please suggest your name.
##########
File path: samza-core/src/main/java/org/apache/samza/util/DiagnosticsUtil.java
##########
@@ -144,7 +145,8 @@ public MetadataFileContents(String version, String metricsSnapshot) {
}
SystemFactory systemFactory = ReflectionUtil.getObj(diagnosticsSystemFactoryName.get(), SystemFactory.class);
SystemProducer systemProducer =
- systemFactory.getProducer(diagnosticsSystemStream.getSystem(), config, new MetricsRegistryMap());
+ systemFactory.getProducer(diagnosticsSystemStream.getSystem(), config, new MetricsRegistryMap(),
+ MethodHandles.lookup().lookupClass().getSimpleName());
Review comment:
I think it is a static method. And we CAN use simpleName(), but we don't have too. As long as it is unique for the caller.
----------------------------------------------------------------
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] prateekm commented on a change in pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466706519
##########
File path: samza-core/src/main/java/org/apache/samza/util/DiagnosticsUtil.java
##########
@@ -144,7 +145,8 @@ public MetadataFileContents(String version, String metricsSnapshot) {
}
SystemFactory systemFactory = ReflectionUtil.getObj(diagnosticsSystemFactoryName.get(), SystemFactory.class);
SystemProducer systemProducer =
- systemFactory.getProducer(diagnosticsSystemStream.getSystem(), config, new MetricsRegistryMap());
+ systemFactory.getProducer(diagnosticsSystemStream.getSystem(), config, new MetricsRegistryMap(),
+ MethodHandles.lookup().lookupClass().getSimpleName());
Review comment:
Got it, thanks. We can probably use `DiagnosticsUtil.class.getSimpleName()` so that any refactoring updates it.
----------------------------------------------------------------
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 #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
sborya commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r467146936
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -27,9 +27,56 @@
* a particular system, as well as the accompanying {@link org.apache.samza.system.SystemAdmin}.
*/
public interface SystemFactory {
+ @Deprecated
SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemAdmin getAdmin(String systemName, Config config);
+
+ /**
+ * This function provides an extra input parameter than {@link #getConsumer}, which can be used to provide extra
+ * information e.g. ownership of client instance, to helper better identify consumers in logs,
+ * threads and client instances etc., along with other relevant information like systemName
+ *
+ * @param systemName The name of the system to create consumer for.
+ * @param config The config to create consumer with.
+ * @param registry MetricsRegistry to which to publish consumer specific metrics.
+ * @param consumerLabel a string used to provide info the consumer instance.
+ * @return A SystemConsumer
+ */
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerLabel) {
+ return getConsumer(systemName, config, registry);
+ }
+
+ /**
+ * This function provides an extra input parameter than {@link #getProducer}, which can be used to provide extra
+ * information e.g. ownership of client instance, to helper better identify producers in logs,
+ * threads and client instances etc., along with other relevant information like systemName
+ *
+ * @param systemName The name of the system to create producer for.
+ * @param config The config to create producer with.
+ * @param registry MetricsRegistry to which to publish producer specific metrics.
+ * @param producerLabel a string used to provide info the producer instance.
+ * @return A SystemProducer
+ */
+ default SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry, String producerLabel) {
+ return getProducer(systemName, config, registry);
+ }
+
+ /**
+ * This function provides an extra input parameter than {@link #getAdmin}, which can be used to provide extra
+ * information e.g. ownership of client instance, to helper better identify admins in logs,
+ * threads and client instances etc., along with other relevant information like systemName
Review comment:
^^^
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -27,9 +27,56 @@
* a particular system, as well as the accompanying {@link org.apache.samza.system.SystemAdmin}.
*/
public interface SystemFactory {
+ @Deprecated
SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemAdmin getAdmin(String systemName, Config config);
+
+ /**
+ * This function provides an extra input parameter than {@link #getConsumer}, which can be used to provide extra
+ * information e.g. ownership of client instance, to helper better identify consumers in logs,
+ * threads and client instances etc., along with other relevant information like systemName
+ *
+ * @param systemName The name of the system to create consumer for.
+ * @param config The config to create consumer with.
+ * @param registry MetricsRegistry to which to publish consumer specific metrics.
+ * @param consumerLabel a string used to provide info the consumer instance.
Review comment:
" a string to provide extra info for the consumer instance."
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -27,9 +27,56 @@
* a particular system, as well as the accompanying {@link org.apache.samza.system.SystemAdmin}.
*/
public interface SystemFactory {
+ @Deprecated
SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemAdmin getAdmin(String systemName, Config config);
+
+ /**
+ * This function provides an extra input parameter than {@link #getConsumer}, which can be used to provide extra
+ * information e.g. ownership of client instance, to helper better identify consumers in logs,
+ * threads and client instances etc., along with other relevant information like systemName
Review comment:
nit. add '.' at the end.
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -27,9 +27,56 @@
* a particular system, as well as the accompanying {@link org.apache.samza.system.SystemAdmin}.
*/
public interface SystemFactory {
+ @Deprecated
SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
+ @Deprecated
SystemAdmin getAdmin(String systemName, Config config);
+
+ /**
+ * This function provides an extra input parameter than {@link #getConsumer}, which can be used to provide extra
+ * information e.g. ownership of client instance, to helper better identify consumers in logs,
+ * threads and client instances etc., along with other relevant information like systemName
+ *
+ * @param systemName The name of the system to create consumer for.
+ * @param config The config to create consumer with.
+ * @param registry MetricsRegistry to which to publish consumer specific metrics.
+ * @param consumerLabel a string used to provide info the consumer instance.
+ * @return A SystemConsumer
+ */
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerLabel) {
+ return getConsumer(systemName, config, registry);
+ }
+
+ /**
+ * This function provides an extra input parameter than {@link #getProducer}, which can be used to provide extra
+ * information e.g. ownership of client instance, to helper better identify producers in logs,
+ * threads and client instances etc., along with other relevant information like systemName
+ *
+ * @param systemName The name of the system to create producer for.
+ * @param config The config to create producer with.
+ * @param registry MetricsRegistry to which to publish producer specific metrics.
+ * @param producerLabel a string used to provide info the producer instance.
Review comment:
see above.
----------------------------------------------------------------
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] prateekm commented on a change in pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466760235
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
@MabelYC what do you mean? I'm suggesting that we add the annotation to mark them as deprecated (e.g. https://www.baeldung.com/java-deprecated). We will not remove the older methods. This will mean that when some code calls the older methods, the author will get build time warnings that the method they're calling is deprecated, so that they can start using the new (better) method instead.
----------------------------------------------------------------
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 #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
sborya commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r465922429
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String callerClass) {
Review comment:
please rename callClass to 'consumerIdPrefix'
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String callerClass) {
+ return getConsumer(systemName, config, registry);
+ }
+
+ default SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry, String callerClass) {
Review comment:
please rename callClass to 'producerIdPrefix'
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String callerClass) {
+ return getConsumer(systemName, config, registry);
+ }
+
+ default SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry, String callerClass) {
+ return getProducer(systemName, config, registry);
+ }
+
+ default SystemAdmin getAdmin(String systemName, Config config, String callerClass) {
Review comment:
please rename callClass to 'adminIdPrefix'
----------------------------------------------------------------
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] prateekm commented on a change in pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466710975
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
Should we mark the other methods as @Deprecated to encourage use of the new methods and identify other call sites that need to be updated?
----------------------------------------------------------------
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 #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
MabelYC commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466771910
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
@prateekm make sense. Marked theme as @deprecated.
----------------------------------------------------------------
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 #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
MabelYC commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466734969
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
this one: https://samza.apache.org/learn/documentation/0.7.0/api/javadocs/org/apache/samza/system/SystemFactory.html ?
----------------------------------------------------------------
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] prateekm commented on a change in pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466760417
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
That's a good point, let's keep this order and add javadocs.
----------------------------------------------------------------
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 #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
sborya commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466563595
##########
File path: samza-core/src/main/java/org/apache/samza/util/DiagnosticsUtil.java
##########
@@ -144,7 +145,8 @@ public MetadataFileContents(String version, String metricsSnapshot) {
}
SystemFactory systemFactory = ReflectionUtil.getObj(diagnosticsSystemFactoryName.get(), SystemFactory.class);
SystemProducer systemProducer =
- systemFactory.getProducer(diagnosticsSystemStream.getSystem(), config, new MetricsRegistryMap());
+ systemFactory.getProducer(diagnosticsSystemStream.getSystem(), config, new MetricsRegistryMap(),
+ MethodHandles.lookup().lookupClass().getSimpleName());
Review comment:
i think it is ok to hardcode the prefix to "DiagnosticsUtil" too.
----------------------------------------------------------------
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 #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
MabelYC commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466734546
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
> Also, let's make this the second parameter after systemName to group naming related parameters together.
I think change order may not be a good option. We are adding functions to help make previous ones more flexible, so may be better to keep the order the same as previous one. And also, systemName is not intended to be a naming parameter. So i think it may be better to keep it in current order. what do you think？
----------------------------------------------------------------
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] prateekm commented on pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on pull request #1408:
URL: https://github.com/apache/samza/pull/1408#issuecomment-669389829
@MabelYC Please also explain (in the description) why this change is required and how this will be used.
----------------------------------------------------------------
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] prateekm commented on a change in pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466710445
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
I'm assuming this is intended to be used as part of the Kafka clientId to figure out who owns the instance?
Maybe consumerLabel. We should add some documentation on what this should be set to by the caller (e.g., set this to indicate ownership of the client instance), how this will be used by system implementers (e.g., to identify consumers in logs, threads and client instances etc., along with other relevant information like systemName).
----------------------------------------------------------------
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] prateekm commented on a change in pull request #1408: SAMZA-2574: improve flexibility of SystemFactory interface
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1408:
URL: https://github.com/apache/samza/pull/1408#discussion_r466670876
##########
File path: samza-api/src/main/java/org/apache/samza/system/SystemFactory.java
##########
@@ -32,4 +32,16 @@
SystemProducer getProducer(String systemName, Config config, MetricsRegistry registry);
SystemAdmin getAdmin(String systemName, Config config);
+
+ default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {
Review comment:
Also, let's make this the second parameter after systemName to group naming related parameters together.
----------------------------------------------------------------
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