You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "smiklosovic (via GitHub)" <gi...@apache.org> on 2023/08/09 22:21:26 UTC
[GitHub] [cassandra] smiklosovic opened a new pull request, #2564: CASSANDRA-18717 5.0 Fix all javadocs errors to be able to build them locally
smiklosovic opened a new pull request, #2564:
URL: https://github.com/apache/cassandra/pull/2564
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #2564: CASSANDRA-18717 5.0 Fix all javadocs errors to be able to build them locally
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #2564:
URL: https://github.com/apache/cassandra/pull/2564#discussion_r1298629969
##########
build.xml:
##########
@@ -305,7 +305,7 @@
<!-- needed to compile org.apache.cassandra.utils.JMXServerUtils -->
<!-- needed to compile org.apache.cassandra.distributed.impl.Instance-->
<!-- needed to compile org.apache.cassandra.utils.memory.BufferPool -->
- <property name="jdk11plus-javac-exports" value="--add-exports java.rmi/sun.rmi.registry=ALL-UNNAMED --add-exports java.rmi/sun.rmi.transport.tcp=ALL-UNNAMED --add-exports java.base/jdk.internal.ref=ALL-UNNAMED --add-exports java.base/sun.nio.ch=ALL-UNNAMED" />
+ <property name="jdk11plus-javac-exports" value="--add-exports java.rmi/sun.rmi.registry=ALL-UNNAMED --add-exports java.rmi/sun.rmi.transport.tcp=ALL-UNNAMED --add-exports java.base/jdk.internal.ref=ALL-UNNAMED --add-exports java.base/sun.nio.ch=ALL-UNNAMED -Xmaxerrs 65536" />
Review Comment:
I think I am fine with the smaller limit, why would we want such a pollution? And technically if we enable in CI the task, we should never go that far.
##########
build.xml:
##########
@@ -334,7 +334,7 @@
<javadoc destdir="@{destdir}" author="true" version="true" use="true"
windowtitle="${ant.project.name} API" classpathref="cassandra.classpath"
bottom="Copyright &copy; 2009- The Apache Software Foundation"
- useexternalfile="yes" encoding="UTF-8" failonerror="false"
+ useexternalfile="yes" encoding="UTF-8" failonerror="true"
Review Comment:
We can make it true if we have it added to ant check task. Until then, I would not recommend to fail artifacts as technically no one is testing them pre-commit and it will be pretty annoying.
##########
src/java/org/apache/cassandra/service/EmbeddedCassandraService.java:
##########
@@ -24,9 +24,9 @@
* This kind of service is useful when running unit tests of
* services using cassandra for example.
*
- * See {@link org.apache.cassandra.service.EmbeddedCassandraServiceTest} for usage.
+ * See {@code org.apache.cassandra.ServerTestUtils#startEmbeddedCassandraService()} for usages.
* <p>
- * This is the implementation of https://issues.apache.org/jira/browse/CASSANDRA-740
+ * This is the implementation of {@code https://issues.apache.org/jira/browse/CASSANDRA-740}
Review Comment:
Why is this not a link?
##########
build.xml:
##########
@@ -334,7 +334,7 @@
<javadoc destdir="@{destdir}" author="true" version="true" use="true"
windowtitle="${ant.project.name} API" classpathref="cassandra.classpath"
bottom="Copyright &copy; 2009- The Apache Software Foundation"
- useexternalfile="yes" encoding="UTF-8" failonerror="false"
+ useexternalfile="yes" encoding="UTF-8" failonerror="true"
Review Comment:
After reviewing the whole patch I am inclined to suggest only big fat warning on broken task and not to fail builds.
I can imagine how that will slow down big CEPs as people will see it most probably in CI when they run check. I would prefer follow up tickets. IDE does not warn you on all potential errors, not sure if we can change that by default for the project? With the current setup this will slow down and annoy people who do big features development.
##########
src/java/org/apache/cassandra/net/InboundMessageHandler.java:
##########
@@ -53,17 +53,19 @@
* thread pool for processing. Large messages accumulate frames until completion of a message, then hand off
* the untouched frames to the correct thread pool for the verb to be deserialized there and immediately processed.
*
- * # Flow control (backpressure)
+ * <h2>Flow control (backpressure)</h2>
*
* To prevent nodes from overwhelming and bringing each other to the knees with more inbound messages that
* can be processed in a timely manner, {@link InboundMessageHandler} implements a strict flow control policy.
* The size of the incoming message is dependent on the messaging version of the specific peer connection. See
- * {@link Message.Serializer#inferMessageSize(ByteBuffer, int, int, int)}.
+ * {@link Message.Serializer#inferMessageSize(ByteBuffer, int, int)}.
*
* By default, every connection has 4MiB of exlusive permits available before needing to access the per-endpoint
* and global reserves.
*
* Permits are released after the verb handler has been invoked on the {@link Stage} for the {@link Verb} of the message.
+ *
+ * @see Message.Serializer#inferMessageSize(ByteBuffer, int, int)
Review Comment:
Again?
##########
src/java/org/apache/cassandra/cql3/Terms.java:
##########
@@ -198,7 +198,7 @@ public List<Terminal> bind(QueryOptions options)
/**
* Creates a {@code Terms} containing a set of {@code Term}.
*
- * @param term the {@code Term}
+ * @param terms the {@code Term}
Review Comment:
Probably we should not just say the` {@code Term} `but that it is a list of terms, similar to `return`
##########
src/java/org/apache/cassandra/auth/CIDRGroupsMappingIntervalTree.java:
##########
@@ -57,10 +57,10 @@
*
* Example:
* Assume below CIDRs
- * "128.10.120.2/10", ==> IP range 128.0.0.0 - 128.63.255.255, netmask 10
- * "128.20.120.2/20", ==> IP range 128.20.112.0 - 128.20.127.255, netmask 20
- * "0.0.0.0/0", ==> IP range 0.0.0.0 - 255.255.255.255, netmask 0
- * "10.1.1.2/10" ==> IP range 10.0.0.0 - 10.63.255.255, netmask 10
+ * {@code "128.10.120.2/10", ==> IP range 128.0.0.0 - 128.63.255.255, netmask 10}
Review Comment:
This was not showing any warning or anything in IDE, failing only when we run the task. Is there anything we can do to improve that if we are going to fail people on this task?
##########
src/java/org/apache/cassandra/repair/consistent/ConsistentSession.java:
##########
@@ -52,75 +52,75 @@
/**
* Base class for consistent Local and Coordinator sessions
*
- * <p/>
+ * <p>
* There are 4 stages to a consistent incremental repair.
*
- * <h1>Repair prepare</h1>
+ * <h2>Repair prepare</h2>
* First, the normal {@link ActiveRepairService#prepareForRepair(TimeUUID, InetAddressAndPort, Set, RepairOption, boolean, List)} stuff
* happens, which sends out {@link PrepareMessage} and creates a {@link ActiveRepairService.ParentRepairSession}
* on the coordinator and each of the neighbors.
*
- * <h1>Consistent prepare</h1>
+ * <h2>Consistent prepare</h2>
* The consistent prepare step promotes the parent repair session to a consistent session, and isolates the sstables
* being repaired from other sstables. First, the coordinator sends a {@link PrepareConsistentRequest} message to each repair
* participant (including itself). When received, the node creates a {@link LocalSession} instance, sets it's state to
* {@code PREPARING}, persists it, and begins a preparing the tables for incremental repair, which segregates the data
* being repaired from the rest of the table data. When the preparation completes, the session state is set to
* {@code PREPARED}, and a {@link PrepareConsistentResponse} is sent to the coordinator indicating success or failure.
* If the pending anti-compaction fails, the local session state is set to {@code FAILED}.
- * <p/>
+ * <p>
* (see {@link LocalSessions#handlePrepareMessage(InetAddressAndPort, PrepareConsistentRequest)}
- * <p/>
+ * <p>
* Once the coordinator recieves positive {@code PrepareConsistentResponse} messages from all the participants, the
* coordinator begins the normal repair process.
- * <p/>
+ * <p>
* (see {@link CoordinatorSession#handlePrepareResponse(InetAddressAndPort, boolean)}
*
- * <h1>Repair</h1>
+ * <h2>Repair</h2>
* The coordinator runs the normal data repair process against the sstables segregated in the previous step. When a
* node recieves a {@link ValidationRequest}, it sets it's local session state to {@code REPAIRING}.
- * <p/>
+ * <p>
*
* If all of the RepairSessions complete successfully, the coordinator begins the {@code Finalization} process. Otherwise,
* it begins the {@code Failure} process.
*
- * <h1>Finalization</h1>
+ * <h2>Finalization</h2>
* The finalization step finishes the session and promotes the sstables to repaired. The coordinator begins by sending
* {@link FinalizePropose} messages to each of the participants. Each participant will set it's state to {@code FINALIZE_PROMISED}
* and respond with a {@link FinalizePromise} message. Once the coordinator has received promise messages from all participants,
* it will send a {@link FinalizeCommit} message to all of them, ending the coordinator session. When a node receives the
* {@code FinalizeCommit} message, it will set it's sessions state to {@code FINALIZED}, completing the {@code LocalSession}.
- * <p/>
+ * <p>
*
* For the sake of simplicity, finalization does not immediately mark pending repair sstables repaired because of potential
* conflicts with in progress compactions. The sstables will be marked repaired as part of the normal compaction process.
- * <p/>
+ * <p>
*
* On the coordinator side, see {@link CoordinatorSession#finalizePropose()}, {@link CoordinatorSession#handleFinalizePromise(InetAddressAndPort, boolean)},
- * & {@link CoordinatorSession#finalizeCommit()}
- * <p/>
+ * and {@link CoordinatorSession#finalizeCommit()}
+ * <p>
*
* On the local session side, see {@link LocalSessions#handleFinalizeProposeMessage(InetAddressAndPort, FinalizePropose)}
- * & {@link LocalSessions#handleFinalizeCommitMessage(InetAddressAndPort, FinalizeCommit)}
+ * and {@link LocalSessions#handleFinalizeCommitMessage(InetAddressAndPort, FinalizeCommit)}
*
- * <h1>Failure</h1>
+ * <h2>Failure</h2>
* If there are any failures or problems during the process above, the session will be failed. When a session is failed,
* the coordinator will send {@link FailSession} messages to each of the participants. In some cases (basically those not
* including Validation and Sync) errors are reported back to the coordinator by the local session, at which point, it
* will send {@code FailSession} messages out.
- * <p/>
+ * <p>
* Just as with finalization, sstables aren't immediately moved back to unrepaired, but will be demoted as part of the
* normal compaction process.
*
- * <p/>
- * See {@link LocalSessions#failSession(UUID, boolean)} and {@link CoordinatorSession#fail()}
+ * <p>
+ * See {@code LocalSessions#failSession(UUID, boolean)} and {@link CoordinatorSession#fail()}
Review Comment:
Why LocalSessions doesn't work with link? I suspect because it cannot resolve for some reason the method
But it is weird to have link and code here
##########
src/java/org/apache/cassandra/db/view/ViewBuilderTask.java:
##########
@@ -30,6 +30,7 @@
import com.google.common.base.Objects;
import com.google.common.collect.Iterators;
import com.google.common.collect.PeekingIterator;
+import com.google.common.util.concurrent.Futures; //checkstyle: permit this import
Review Comment:
I am confused about this change, was it some leftover?
##########
src/java/org/apache/cassandra/auth/CassandraAuthorizer.java:
##########
@@ -454,7 +454,7 @@ public static ConsistencyLevel authReadConsistencyLevel()
/**
* Get an initial set of permissions to load into the PermissionsCache at startup
- * @return map of User/Resource -> Permissions for cache initialisation
+ * @return map of User/Resource {@code ->} Permissions for cache initialisation
Review Comment:
Same with this one
##########
src/java/org/apache/cassandra/service/EmbeddedCassandraService.java:
##########
@@ -24,9 +24,9 @@
* This kind of service is useful when running unit tests of
* services using cassandra for example.
*
- * See {@link org.apache.cassandra.service.EmbeddedCassandraServiceTest} for usage.
Review Comment:
I prefer not to do this change. The test example is ok here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org