MINOR: Kafka Streams code samples formating unification by jlprat · Pull Request #10651 · apache/kafka

jlprat · 2021-05-07T16:36:13Z

Code samples are now correctly formatted.
Samples under Streams use consistently the prism library to be displayed.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

Code samples are now unified and correctly formatted. Samples under Streams use consistently the prism library.

jlprat · 2021-05-07T18:28:41Z

The test failures in this PR should be unrelated to the changes introduced (docs). Is there anything I should do to move the PR forward?

jlprat · 2021-05-10T08:07:37Z

For the record:
The contribution is my original work and that I license the work to the project under the project's open source license.

cadonna · 2021-05-11T09:05:48Z

Thank for the PR!

For the next time, I would recommend to open smaller PRs. Almost 2000 additions is a lot even for docs. In my experience smaller PRs tend to be merged faster.

cadonna · 2021-05-11T09:06:36Z

Do not worry about the test failures. First of all, your PR does not touch any code, so there is no reason a test would fail due to your PR. Additionally, the failing tests are all known to be flaky. You can find known flaky tests in the JIRA by searching for the test name.

jlprat · 2021-05-11T09:07:21Z

@cadonna I'll do it next time. I was doubting between providing a PR per file or a PR per folder. (ended up doing PR for the Streams folder).

jlprat · 2021-05-11T09:09:21Z

Most of the changes are purely cosmetic:

using the right tags for for embedding a code snippet
escaping < and > characters to HTML encoded strings so they are properly rendered
removing extra indentation

jlprat · 2021-05-14T10:41:47Z

@cadonna Would you like me to split this PR into several ones? One per file, for example?

cadonna

@jlprat Thank you for the PR!

I did a pass over the first 5 files. I will continue in the next days.

Here my feedback up to now.

cadonna · 2021-05-18T09:24:38Z

docs/streams/developer-guide/app-reset-tool.html

+            <pre class="line-numbers"><code class="language-bash">&lt;path-to-kafka&gt;/bin/kafka-streams-application-reset</code></pre>
            <p>The tool accepts the following parameters:</p>
-            <div class="highlight-bash"><div class="highlight"><pre><span>Option</span><code> <span class="o">(</span>* <span class="o">=</span> required<span class="o">)</span>                 Description
+            <pre class="line-numbers"><code class="language-bash">Option (* = required)                 Description


Class language-bash renders the text with strange colors, e.g., the keyword "file" which is not a keyword here. Since this is actually not a bash script code, I think we leave it as plain monospaced text.

Fixed, and also aligned the spaces, so it fit's to the 2 column printing.

cadonna · 2021-05-18T13:52:21Z

docs/streams/developer-guide/dsl-api.html

+
+// Java 8+ example, using lambda expressions
+                KTable&lt;String, String&gt; joined = left.join(right,
+    foreignKeyExtractor,


Could you please put foreignKeyExtractor on the previous line or indent it so that it is at the same column as right?

cadonna · 2021-05-18T13:52:34Z

docs/streams/developer-guide/dsl-api.html

+
+// Java 8+ example, using lambda expressions
+                KTable&lt;String, String&gt; joined = left.join(right,
+    foreignKeyExtractor,


Could you please put foreignKeyExtractor on the previous line or indent it so that it is at the same column as right?

cadonna · 2021-05-18T13:55:26Z

docs/streams/developer-guide/dsl-api.html

+// A hopping time window with a size of 5 minutes and an advance interval of 1 minute.
+// The window&#39;s name -- the string parameter -- is used to e.g. name the backing state store.
+Duration windowSizeMs = Duration.ofMinutes(5);
+Duration advanceMs =    Duration.ofMinutes(1);


Could you fix the indentation here to Duration advanceMs = Duration.ofMinutes(1);?

jlprat · 2021-05-18T14:13:14Z

Thanks @cadonna, all good points. I'll address the feedback later

cadonna

Thank you for the updates, @jlprat !

I reviewed the files until and including security.html.

Here my feedback.

cadonna · 2021-05-21T08:24:39Z

docs/streams/developer-guide/memory-mgmt.html

-
-       <span class="kd">private</span> <span class="kt">static</span> <span class="n">org.rocksdb.Cache</span> <span class="n">cache</span> <span class="o">=</span> <span class="k">new</span> <span class="n">org</span><span class="o">.</span><span class="na">rocksdb</span><span class="o">.</span><span class="na">LRUCache</span><span class="o">(</span><span class="mi">TOTAL_OFF_HEAP_MEMORY</span><span class="o">,</span> <span class="n">-1</span><span class="o">,</span> <span class="n">false</span><span class="o">,</span> <span class="n">INDEX_FILTER_BLOCK_RATIO</span><span class="o">);</span><sup><a href="#fn1" id="ref1">1</a></sup>
-       <span class="kd">private</span> <span class="kt">static</span> <span class="n">org.rocksdb.WriteBufferManager</span> <span class="n">writeBufferManager</span> <span class="o">=</span> <span class="k">new</span> <span class="n">org</span><span class="o">.</span><span class="na">rocksdb</span><span class="o">.</span><span class="na">WriteBufferManager</span><span class="o">(</span><span class="mi">TOTAL_MEMTABLE_MEMORY</span><span class="o">,</span> cache<span class="o">);</span>
+   private static org.rocksdb.Cache cache = new org.rocksdb.LRUCache(TOTAL_OFF_HEAP_MEMORY, -1, false, INDEX_FILTER_BLOCK_RATIO);1


The 1 at the end should be a superscript reference to a footnote. Is there a way to get again a superscript here? If not what would be the alternatives?

cadonna · 2021-05-21T08:25:15Z

docs/streams/developer-guide/memory-mgmt.html

-         <span class="n">options</span><span class="o">.</span><span class="na">setMaxWriteBufferNumber</span><span class="o">(</span><span class="mi">N_MEMTABLES</span><span class="o">);</span>
-         <span class="n">options</span><span class="o">.</span><span class="na">setWriteBufferSize</span><span class="o">(</span><span class="mi">MEMTABLE_SIZE</span><span class="o">);</span>
+      // These options are recommended to be set when bounding the total memory
+     tableConfig.setCacheIndexAndFilterBlocksWithHighPriority(true);2


See my comment above for the 2 at the end.

cadonna · 2021-05-21T08:25:23Z

docs/streams/developer-guide/memory-mgmt.html

+      // These options are recommended to be set when bounding the total memory
+     tableConfig.setCacheIndexAndFilterBlocksWithHighPriority(true);2
+     tableConfig.setPinTopLevelIndexAndFilter(true);
+     tableConfig.setBlockSize(BLOCK_SIZE);3


See my comment above for the 3 at the end.

jlprat · 2021-05-21T08:53:20Z

Thanks @cadonna that one slipped through. I modified the code and brought back the foot notes with the hyperlink.

cadonna

@jlprat Hi again!

I reviewed all files!

I found two code snippets that slipped through.

The rest looks good!

cadonna · 2021-05-21T13:24:58Z

docs/streams/developer-guide/testing.html

            <p>
                The mock will capture any values that your processor forwards. You can make assertions on them:
-            <pre class="line-numbers"><code class="language-text">processorUnderTest.process("key", "value");
+            <pre class="line-numbers"><code class="language-java">processorUnderTest.process("key", "value");


On line 304 and line 306 are two code snippets that slipped through.

jlprat · 2021-05-21T13:35:03Z

Hi @cadonna I'll fix that in a couple of hours, thanks for your thorough review!

cadonna

Thanks @jlprat !

LGTM!

jlprat · 2021-05-21T15:33:17Z

Thanks for the review, I know it was long and tricky!

…e-allocations-lz4 * apache-github/trunk: (43 commits) KAFKA-12800: Configure generator to fail on trailing JSON tokens (apache#10717) MINOR: clarify message ordering with max in-flight requests and idempotent producer (apache#10690) MINOR: Add log identifier/prefix printing in Log layer static functions (apache#10742) MINOR: update java doc for deprecated methods (apache#10722) MINOR: Fix deprecation warnings in SlidingWindowedCogroupedKStreamImplTest (apache#10703) KAFKA-12499: add transaction timeout verification (apache#10482) KAFKA-12620 Allocate producer ids on the controller (apache#10504) MINOR: Kafka Streams code samples formating unification (apache#10651) KAFKA-12808: Remove Deprecated Methods under StreamsMetrics (apache#10724) KAFKA-12522: Cast SMT should allow null value records to pass through (apache#10375) KAFKA-12820: Upgrade maven-artifact dependency to resolve CVE-2021-26291 HOTFIX: fix checkstyle issue in KAFKA-12697 KAFKA-12697: Add OfflinePartitionCount and PreferredReplicaImbalanceCount metrics to Quorum Controller (apache#10572) KAFKA-12342: Remove MetaLogShim and use RaftClient directly (apache#10705) KAFKA-12779: KIP-740, Clean up public API in TaskId and fix TaskMetadata#taskId() (apache#10735) KAFKA-12814: Remove Deprecated Method StreamsConfig getConsumerConfigs (apache#10737) MINOR: Eliminate redundant functions in LogTest suite (apache#10732) MINOR: Remove unused maxProducerIdExpirationMs parameter in Log constructor (apache#10723) MINOR: Updating files with release 2.7.1 (apache#10660) KAFKA-12809: Remove deprecated methods of Stores factory (apache#10729) ...

We have updated the memory-mgmt.html in kafka repo in this PR: apache/kafka#10651. So, this pr, just simply fix the missing pre closing tag issue.

MINOR: Kafka Streams code samples formating unification

0921a1f

Code samples are now unified and correctly formatted. Samples under Streams use consistently the prism library.

cadonna reviewed May 18, 2021

View reviewed changes

Apply feedback on formatting

7007c0e

cadonna reviewed May 21, 2021

View reviewed changes

Add foot notes code

1e9cdc0

cadonna reviewed May 21, 2021

View reviewed changes

Set the java syntax highlight on missing snippets

7dbb837

jlprat requested a review from cadonna May 21, 2021 15:05

cadonna approved these changes May 21, 2021

View reviewed changes

cadonna merged commit aa25176 into apache:trunk May 21, 2021

jlprat deleted the MINOR-stream-code-samples-formatting branch May 21, 2021 15:32

showuon mentioned this pull request Jun 25, 2021

KAFKA-12993: fix memory-mgmt.html formatting apache/kafka-site#361

Merged

Conversation

jlprat commented May 7, 2021

Committer Checklist (excluded from commit message)

Uh oh!

jlprat commented May 7, 2021

Uh oh!

jlprat commented May 10, 2021

Uh oh!

cadonna commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cadonna commented May 11, 2021

Uh oh!

jlprat commented May 11, 2021

Uh oh!

jlprat commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlprat commented May 14, 2021

Uh oh!

cadonna 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!

Choose a reason for hiding this comment

Uh oh!

jlprat commented May 18, 2021

Uh oh!

cadonna 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!

jlprat commented May 21, 2021

Uh oh!

cadonna 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!

jlprat commented May 21, 2021

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

jlprat commented May 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cadonna commented May 11, 2021 •

edited

Loading

jlprat commented May 11, 2021 •

edited

Loading