Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SAMZA-1268: Javadoc cleanup for public APIs for 0.13 release #169

Closed
wants to merge 3 commits into from

Conversation

prateekm
Copy link
Contributor

@prateekm prateekm commented May 8, 2017

This PR cleans up javadocs for Fluent API classes in the samza-api module.
Also updates the TaskContext (existing) and ContextManager (new) interfaces to add support for setting an pass-through user-defined context.

@prateekm
Copy link
Contributor Author

prateekm commented May 8, 2017

@vjagadish1989 @jmakes @bharathkk, please let me know if there's anything else we can clarify.

Copy link
Contributor

@jmakes jmakes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better, thanks.
Some minor stuff below.

@@ -163,23 +169,25 @@ private Windows() { }
*
* <pre> {@code
* MessageStream<String> stream = ...;
* BiFunction<String, Integer, Integer> maxAggregator = (m, c)-> Math.max(parseInt(m), c);
* Supplier<Integer> initialValue = () -> 0;
* FoldLeftFunction<String, Integer, Integer> maxAggregator = (m, c) -> Math.max(parseInt(m), c);
* MessageStream<WindowPane<Void, Integer>> windowedStream = stream.window(
* Windows.tumblingWindow(Duration.ofSeconds(10), maxAggregator));
* }
* </pre>
*
* @param duration the duration in processing time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Why is this parameter named differently for keyed and unkeyed tumbling windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed.

*/
@InterfaceStability.Unstable
public interface InitableFunction {

/**
* Interface method to initialize the context for a specific message transformation function.
* Initializes the function before any messages are processed.
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any guarantees we can provide about the order in which init() is invoked if there are multiple InitableFunctions in a StreamApplication?

It'd be nice to inform the users if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not currently. If there are multiple input streams, it'll depend on the iteration order for the Map that contains them.

That said, we should try to guarantee a deterministic and predictable iteration order if possible. Created SAMZA-1271 to track.

*
* @param message the incoming message that is added to the window. This object should not be mutated.
* @param oldValue the previous value
* @param message the message being to the window
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

being added

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -22,7 +22,8 @@


/**
* A function that specifies whether a message should be retained for further processing or filtered out.
* Specifies whether a message should be retained for further processing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a java Predicate impl be used interchangeably with this interface? If so, that'd be pretty powerful and worth mentioning.

Top of my head, we'd need this interface to extend Predicate, so the answer is probably no. Just throwing it out there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, cannot be used interchangeably.
We discussed using java types for these functions (Predicate, Function, BiFunction etc.) instead of introducing our own earlier (see https://github.com/apache/samza/pull/51/files/001be632dcfdbbb138b792265b991722a2ea5597#diff-e1f7020353d9bd97ac274b6e3607d5e3 for more context). TL;DR currently these give us some flexibility to add marker interfaces or behavior (Initable, Closeable, Serializable) to these functions if we need it. For users that just use a lambda the difference shouldn't be noticeable.
I think we have a clearer expectations from these functions now, so we can discuss this again when @nickpan47's back.

* emitted as matches are found.
* <p>
* <b>Note:</b> Currently messages in joins are kept in memory and may be lost in case of failures.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to date this note with a release version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -100,6 +100,8 @@
* {@link WindowPane}s.
* <p>
* Use the {@link org.apache.samza.operators.windows.Windows} helper methods to create the appropriate windows.
* <p>
* <b>Note:</b> Currently messages in windows are kept in memory and may be lost in case of failures.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to date this note with a release version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

* {@link #sendTo(OutputStream)} instead. This transform should only be used to output to
* non-stream systems (e.g., an external database).
* <p>
* Offers more control over processing and sending messages than {@link #sendTo(OutputStream)} since
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very helpful note, thanks.

Might also be worth mentioning that this can also be used to send output on a channel that doesn't have a System impl

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@asfgit asfgit closed this in 634e568 May 9, 2017
@prateekm prateekm deleted the api-docs-cleanup branch May 9, 2017 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants