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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
