Skip to content

Conversation

@uce
Copy link

@uce uce commented Jun 30, 2020

Really helpful documentation! Yesterday, I read over most of the docs and found a few typos, including some typos in code. Feel free to push back on any of the changes that seem unnecessary to you.

A few nit comments:

  • In the project setup page, we state that mvn clean package produces an uber JAR which is not true. Is the intention to provide a Maven configuration that does create an uber JAR or are the docs mistaken?
  • In the Python walk-through, we first write a function without a known type and then show how to specify a type. I'm wondering we should only show the known type example and keep it as a note that it's also possible to the use the Any type.
  • Is RocksDB a strict requirement or can users configure any state backend? If RocksDB is a requirement, we could consider pointing this out in https://github.com/apache/flink-statefun/blob/8376afa/docs/concepts/logical.md#function-lifecycle (last sentence).
  • In the Java SDK, we mix tab and space for code formatting. I think spaces are easier to read in the docs (I understand that the code should follow the apache/flink style guide).
  • In the Java SDK async requests section, we could use a stateful match function to reduce some of the boilerplate.
  • Overall, message routing still felt a bit mysterious after reading over the docs. I don't know if others agree, but we might want to add more details on this part of the system.

Ufuk Celebi added 11 commits June 30, 2020 10:18
* Fix typos
* Make snippets consistent
* Add link to Python SDK in further work section
As far as I can tell, flink-statefun is only available under ververica in
DockerHub.
…-blocks.md

These sections are treated as bullet points about stateful functions. Thus, it
feels more natural to have them as subsections.
* Fix typos
* Add link to Java SDK in further work section
@uce uce requested review from sjwiesman and tzulitai June 30, 2020 09:18
Ufuk Celebi added 2 commits July 17, 2020 10:52
Applies Seth's suggestion from the review.
@uce
Copy link
Author

uce commented Jul 17, 2020

@sjwiesman @igalshilman @tzulitai I reverted the Docker image repository change and applied Seth's suggestion for the function types. Can you take another look and merge or close this PR?

@tzulitai
Copy link
Contributor

Thanks @uce, will merge this!

@tzulitai tzulitai closed this Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants