-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
MINOR: Java and TLS documentation improvements #18822
base: trunk
Are you sure you want to change the base?
Conversation
@@ -1527,7 +1527,7 @@ project(':group-coordinator') { | |||
|
|||
|
|||
project(':test-common:test-common-internal-api') { | |||
// Interfaces, config classes, and other test APIs. Java 17 only | |||
// Interfaces, config classes, and other test APIs. Java 17 is the minimum Java version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mumrah I think "Java 17 only" doesn't make it clear that it's the minimum version - thoughts?
@@ -269,7 +269,7 @@ | |||
interface to get access to the underlying instances of your store. | |||
<code class="docutils literal"><span class="pre">StateStoreProvider#stores(String</span> <span class="pre">storeName,</span> <span class="pre">QueryableStoreType<T></span> <span class="pre">queryableStoreType)</span></code> returns a <code class="docutils literal"><span class="pre">List</span></code> of state | |||
stores with the given storeName and of the type as defined by <code class="docutils literal"><span class="pre">queryableStoreType</span></code>.</p> | |||
<p>Here is an example implementation of the wrapper follows (Java 8+):</p> | |||
<p>Here is an example implementation of the wrapper follows (Java 11+):</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjsax Can you please review this and other streams changes in this PR? I went with 11+ since that's the case for general Kafka Streams applications, but I did not check the specifics for the wrapper. Same for other similar cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am frankly not sure why we do need have a version to begin with? Should we just remove it entirely?
I think, back in the days, while we still supported Java7, we did call out example written in Java8, but this pattern does not apply any longer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can do that.
@@ -70,7 +70,7 @@ <h4 class="anchor-heading"><a id="tutorial_maven_setup" class="anchor-link"></a> | |||
|
|||
<p> | |||
The <code>pom.xml</code> file included in the project already has the Streams dependency defined. | |||
Note, that the generated <code>pom.xml</code> targets Java 8, and does not work with higher Java versions. | |||
Note, that the generated <code>pom.xml</code> targets Java 11. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjsax I assume we fixed this so it works with Java 11 or newer - is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we did update the archetype. Good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall I remove the Java version from here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Not sure. Might be ok to leave for now 🤷
public static final String SSL_PROTOCOL_DOC = "The SSL protocol used to generate the SSLContext. The default is 'TLSv1.3', " | ||
+ "which should be fine for most use cases. A typical alternative to the default is 'TLSv1.2'. Allowed values for " | ||
+ "this config are dependent on the JVM. " | ||
+ "Clients using the defaults for this config and 'ssl.enabled.protocols' will downgrade to 'TLSv1.2' if " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I was here, I clarified this note too.
<p> | ||
Java 11 and later versions perform significantly better if TLS is enabled, so they are highly recommended (they also include a number of other | ||
performance improvements: G1GC, CRC32C, Compact Strings, Thread-Local Handshakes and more). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems we should still clarify that Java 11 is not supported for brokers any longer, but only for clients/connect/ks ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I can do that.
@@ -151,7 +151,7 @@ <h2>Using Kafka Streams within your application code<a class="headerlink" href=" | |||
For more information, see <a class="reference internal" href="../architecture.html#streams_architecture_tasks"><span class="std std-ref">Stream Partitions and Tasks</span></a> and <a class="reference internal" href="../architecture.html#streams_architecture_threads"><span class="std std-ref">Threading Model</span></a>.</p> | |||
<p>To catch any unexpected exceptions, you can set an <code class="docutils literal"><span class="pre">java.lang.Thread.UncaughtExceptionHandler</span></code> before you start the | |||
application. This handler is called whenever a stream thread is terminated by an unexpected exception:</p> | |||
<pre class="line-numbers"><code class="language-java">// Java 8+, using lambda expressions | |||
<pre class="line-numbers"><code class="language-java">// Java 11+, using lambda expressions | |||
streams.setUncaughtExceptionHandler((Thread thread, Throwable throwable) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove this comment from the code snippet. If we do, for proper formatting, we need to move this line I add the comment on, to the line above:
<pre class="line-numbers"><code class="language-java">streams.setUncaughtExceptionHandler((Thread thread, Throwable throwable) -> {
@@ -193,7 +193,7 @@ <h3 style="margin-top: 5.3rem;">Hello Kafka Streams</h3> | |||
|
|||
<div class="code-example"> | |||
<div class="btn-group"> | |||
<a class="selected b-java-8" data-section="java-8">Java 8+</a> | |||
<a class="selected b-java-8" data-section="java-8">Java 11+</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<a class="selected b-java-8" data-section="java-8">Java 11+</a> | |
<a class="selected b-java-8" data-section="java-8">Java</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what class="selected b-java-8" data-section="java-8"
is -- do you know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I also wondered what that is. :)
Co-authored-by: Matthias J. Sax <[email protected]>
Co-authored-by: Matthias J. Sax <[email protected]>
Committer Checklist (excluded from commit message)