SAMZA-2574: improve flexibility of SystemFactory interface by MabelYC · Pull Request #1408 · apache/samza

MabelYC · 2020-08-04T01:37:41Z

improve SystemFactory interface by providing user-defined clientId prefix so that we can access more information of the clients created: with changes, user can add specific clientId prefix for clients to help monitor clients.

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: To use the new default functions, user will need to redefine them in their implementation class. And then user can set specific clientId.

Merge from origin branch

samza-api/src/main/java/org/apache/samza/system/SystemFactory.java

prateekm · 2020-08-05T18:39:56Z

@cameronlee314 can you take a look?

prateekm · 2020-08-05T18:40:54Z

@MabelYC Please also explain (in the description) why this change is required and how this will be used.

MabelYC · 2020-08-05T20:50:55Z

@MabelYC Please also explain (in the description) why this change is required and how this will be used.

Sure.

sborya

Please change the name of the rb to describe the changes, not the generic intent. Something to the effect of "..allow specify prefix for consumer/producer/admin ids.."

sborya · 2020-08-06T17:16:59Z

samza-core/src/main/java/org/apache/samza/util/DiagnosticsUtil.java

      SystemProducer systemProducer =
-          systemFactory.getProducer(diagnosticsSystemStream.getSystem(), config, new MetricsRegistryMap());
+          systemFactory.getProducer(diagnosticsSystemStream.getSystem(), config, new MetricsRegistryMap(),
+              MethodHandles.lookup().lookupClass().getSimpleName());


i think it is ok to hardcode the prefix to "DiagnosticsUtil" too.

Why not use Class.getSimpleName() here too?

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.

Got it, thanks. We can probably use DiagnosticsUtil.class.getSimpleName() so that any refactoring updates it.

Got it. Will do.

prateekm · 2020-08-06T20:32:28Z

samza-api/src/main/java/org/apache/samza/system/SystemFactory.java


  SystemAdmin getAdmin(String systemName, Config config);
+
+  default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {


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?

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.

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

by "add some documentation" you mean add javadoc or open source documentation?

this one: https://samza.apache.org/learn/documentation/0.7.0/api/javadocs/org/apache/samza/system/SystemFactory.html ?

@MabelYC I mean Javadocs. The page you linked is auto-generated from Javadocs.

prateekm · 2020-08-06T20:33:18Z

samza-api/src/main/java/org/apache/samza/system/SystemFactory.java


  SystemAdmin getAdmin(String systemName, Config config);
+
+  default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {


Please add method Javadocs clarifying what the parameters are, and how these methods are different than the ones above.

Also, let's make this the second parameter after systemName to group naming related parameters together.

good point. Thanks. Will do.

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？

That's a good point, let's keep this order and add javadocs.

prateekm · 2020-08-06T22:04:39Z

samza-api/src/main/java/org/apache/samza/system/SystemFactory.java


  SystemAdmin getAdmin(String systemName, Config config);
+
+  default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {


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?

Not sure if it is better. With deprecated, user will still have to implement them later. Can it work as we expect?

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

@prateekm make sense. Marked theme as @deprecated.

sborya

with some nit comments.

sborya · 2020-08-07T16:22:06Z

samza-api/src/main/java/org/apache/samza/system/SystemFactory.java

+  /**
+   * 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


nit. add '.' at the end.

sborya · 2020-08-07T16:32:49Z

samza-api/src/main/java/org/apache/samza/system/SystemFactory.java

+   * @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.


" a string to provide extra info for the consumer instance."

sborya · 2020-08-07T16:33:18Z

samza-api/src/main/java/org/apache/samza/system/SystemFactory.java

+   * @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.


sborya · 2020-08-07T16:33:42Z

samza-api/src/main/java/org/apache/samza/system/SystemFactory.java

+  /**
+   * 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


sborya · 2020-08-12T17:17:44Z

samza-api/src/main/java/org/apache/samza/system/SystemFactory.java

+
+  /**
+   * 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,


s/helper/help

sborya · 2020-08-12T17:18:13Z

samza-api/src/main/java/org/apache/samza/system/SystemFactory.java

+
+  /**
+   * 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,


sborya · 2020-08-12T17:19:03Z

samza-api/src/main/java/org/apache/samza/system/SystemFactory.java

+
+  /**
+   * 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,


MabelYC and others added 2 commits August 3, 2020 17:31

Merge pull request #1 from apache/master

e7d3067

Merge from origin branch

SAMZA-2574:improve flexibility of SystemFactory interface

984978b

sborya reviewed Aug 5, 2020

View reviewed changes

rename the input parameter

2092530

sborya reviewed Aug 6, 2020

View reviewed changes

prateekm reviewed Aug 6, 2020

View reviewed changes

MabelYC added 2 commits August 6, 2020 17:24

address comment: adding javadoc and change to a more reasonable name

a05c6e4

clean up comments and code

dcbdf89

MabelYC requested a review from sborya August 7, 2020 01:32

sborya approved these changes Aug 7, 2020

View reviewed changes

MabelYC added 2 commits August 7, 2020 10:10

clean up comments

7aa6580

address comments and provide better comments

10d5d2f

sborya reviewed Aug 12, 2020

View reviewed changes

address comment: clean typo

926c758

sborya approved these changes Aug 12, 2020

View reviewed changes

sborya merged commit b736af9 into apache:master Aug 12, 2020

MabelYC deleted the interfaceChange branch September 16, 2020 01:29


		SystemAdmin getAdmin(String systemName, Config config);

		default SystemConsumer getConsumer(String systemName, Config config, MetricsRegistry registry, String consumerIdPrefix) {

Conversation

MabelYC commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prateekm commented Aug 5, 2020

Uh oh!

prateekm commented Aug 5, 2020

Uh oh!

MabelYC commented Aug 5, 2020

Uh oh!

sborya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prateekm Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sborya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

MabelYC commented Aug 4, 2020 •

edited

Loading

prateekm Aug 6, 2020 •

edited

Loading