Re: Review Request 50174: SAMZA-977: User doc for samza multithreading

[Xinyu Liu <xi...@gmail.com>]

> On Aug. 31, 2016, 12:54 a.m., Navina Ramesh wrote:
> > docs/learn/documentation/versioned/api/overview.md, line 49
> > <https://reviews.apache.org/r/50174/diff/3/?file=1488350#file1488350line49>
> >
> >     // fire callback after process complete 
> >     >> Is the user expected to invoke the callback here or is it samza that invokes the callback ? Not clear from the example. If the user has to invoke the callback, you can write the code snippet. 
> >     
> >     In your previous diff, you had this line - "To complete the process, you need to call callback.complete() or callback.failure(). Samza will continue to process next message or shut down the job based on the callback status." . Is that still valid?

oh, it's still there. I moved it to the sentence before the async task example :).


> On Aug. 31, 2016, 12:54 a.m., Navina Ramesh wrote:
> > docs/learn/documentation/versioned/container/event-loop.md, line 28
> > <https://reviews.apache.org/r/50174/diff/3/?file=1488351#file1488351line28>
> >
> >     "any state change from one operation will be fully visible to others"?  -> does this mean tasks share state (which in my opinion can be anything from shared variables to KV stores). What state change is made fully visible here? 
> >     I think it is sufficient to say that these operatios within a task partition are mutually exclusive. You can probably remove the state change part.

Chris recommended this: it's actually pretty important for the user to know the visibility rules when using AsyncStreamTask.


> On Aug. 31, 2016, 12:54 a.m., Navina Ramesh wrote:
> > docs/learn/documentation/versioned/container/event-loop.md, line 56
> > <https://reviews.apache.org/r/50174/diff/3/?file=1488351#file1488351line56>
> >
> >     It feels like we are introducing new terms here "serialized mode". I think it is sufficient to simply state, if task.max.concurrency = 1, then each message process completion ...
> >     
> >     Also "it is required to coordinate any shared state" can be re-phrased to "user should synchronize access to any shared/global variables in the Task."

Good suggestions. I remove the serialized mode notion.


> On Aug. 31, 2016, 12:54 a.m., Navina Ramesh wrote:
> > docs/learn/documentation/versioned/jobs/configuration-table.html, line 357
> > <https://reviews.apache.org/r/50174/diff/3/?file=1488352#file1488352line357>
> >
> >     Is it too late to comment on the config key pattern? Traditionally, we don't have any strict conventions for config names listed anywhere. Since we use period as a delimiter to distinguish configs belonging to a namespace, I think we should try to avoid using the same in the actual config part. 
> >     For example, job.container.single.thread.mode can be job.container.single-thread-mode.
> >     Same for job.container.thread.pool.size, task.callback.timeout.ms and task.max.concurrency

This config is pretty awkward (so is the name). It's just for fallback to old runloop if there is any issue with the new AsyncRunLoop. So I don't expect users to set it normally. Once the AsyncRunLoop stablized, I will remove this config as well as the old runLoop.


- Xinyu


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50174/#review147388
-----------------------------------------------------------


On Aug. 30, 2016, 12:49 a.m., Xinyu Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50174/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2016, 12:49 a.m.)
> 
> 
> Review request for samza, Chris Pettitt, Navina Ramesh, and Yi Pan (Data Infrastructure).
> 
> 
> Repository: samza
> 
> 
> Description
> -------
> 
> Update samza web docs with new multithreading api, core and configs.
> 
> 
> Diffs
> -----
> 
>   docs/learn/documentation/versioned/api/overview.md 6712344e84e19883b857e00549db2acb101c7e0e 
>   docs/learn/documentation/versioned/container/event-loop.md 116238312df7071747cbbc14bc9c46f558755195 
>   docs/learn/documentation/versioned/jobs/configuration-table.html 54c52981c3055b398ee60af50eeaf2592ed0e64f 
> 
> Diff: https://reviews.apache.org/r/50174/diff/
> 
> 
> Testing
> -------
> 
> Test the web pages locally.
> 
> 
> Thanks,
> 
> Xinyu Liu
> 
>