IgGusev commented on code in PR #1808:
URL: https://github.com/apache/ignite-3/pull/1808#discussion_r1144879798
##########
modules/api/src/main/java/org/apache/ignite/Ignition.java:
##########
@@ -20,6 +20,7 @@
import java.io.InputStream;
import java.net.URL;
import java.nio.file.Path;
+import java.util.Collection;
Review Comment:
Who do we add an import?
##########
modules/api/src/main/java/org/apache/ignite/IgnitionManager.java:
##########
@@ -128,23 +127,59 @@ public static void stop(String nodeName, @Nullable
ClassLoader clsLdr) {
}
/**
- * Initializes the cluster that this node is present in.
+ * Initializes the cluster the specified node belongs to.
+ *
+ * @param nodeName Name of the node the initialization request will be
sent to.
+ * @param metaStorageNodeNames Names of the nodes that will host Meta
Storage and CMG.
+ * @param clusterName Human-readable name of the cluster.
+ * @throws IgniteException If the specified node has not been started or
has been stopped.
+ * @throws NullPointerException If any of the parameters are null.
+ * @throws IllegalArgumentException If {@code metaStorageNodeNames} is
empty or contains blank strings.
+ * @throws IllegalArgumentException If {@code clusterName} is blank.
+ * @see Ignition#init(String, Collection, String)
+ */
+ public static synchronized void init(String nodeName, Collection<String>
metaStorageNodeNames, String clusterName) {
Review Comment:
This looks like a new class, should it be here?
##########
modules/api/src/main/java/org/apache/ignite/sql/BatchedArguments.java:
##########
@@ -78,3 +74,4 @@ public BatchedArguments add(Object... args) {
return this;
}
}
+//"Batched arguments" or "Batch arguments"? Also, do all these methods
"create"? Or do some of them "return"?
Review Comment:
@returns return, @params create it seems
##########
modules/api/src/main/java/org/apache/ignite/sql/async/AsyncResultSet.java:
##########
@@ -49,96 +46,97 @@
* }
* </code></pre>
*
- * @param <T> A type of the objects contained by this result set (when row set
is present). This will be either {@link SqlRow}
- * if no explicit mapper is provided or a particular type defined by
supplied mapper.
- *
* @see ResultSet
- * @see Session#executeAsync(Transaction, String, Object...)
- * @see Session#executeAsync(Transaction, Mapper, String, Object...)
*/
-public interface AsyncResultSet<T> {
+public interface AsyncResultSet {
/**
- * Returns metadata for the results if the result contains rows ({@link
#hasRowSet()} returns {@code true}), or {@code null} if
- * inapplicable.
+ * Returns metadata for query results. If the result set contains rows
({@link #hasRowSet()}, returns {@code true}).
+ * If not applicable, returns {@code null}.
*
- * @return ResultSet metadata.
+ * @return ResultSet Metadata.
* @see ResultSet#metadata()
*/
@Nullable ResultSetMetadata metadata();
+//What does "if not applicable" mean in the above description?
Review Comment:
Just say otherwise, should be correct.
##########
modules/api/src/main/java/org/apache/ignite/sql/async/AsyncResultSet.java:
##########
@@ -49,96 +46,97 @@
* }
* </code></pre>
*
- * @param <T> A type of the objects contained by this result set (when row set
is present). This will be either {@link SqlRow}
- * if no explicit mapper is provided or a particular type defined by
supplied mapper.
- *
* @see ResultSet
- * @see Session#executeAsync(Transaction, String, Object...)
- * @see Session#executeAsync(Transaction, Mapper, String, Object...)
*/
-public interface AsyncResultSet<T> {
+public interface AsyncResultSet {
/**
- * Returns metadata for the results if the result contains rows ({@link
#hasRowSet()} returns {@code true}), or {@code null} if
- * inapplicable.
+ * Returns metadata for query results. If the result set contains rows
({@link #hasRowSet()}, returns {@code true}).
+ * If not applicable, returns {@code null}.
*
- * @return ResultSet metadata.
+ * @return ResultSet Metadata.
* @see ResultSet#metadata()
*/
@Nullable ResultSetMetadata metadata();
+//What does "if not applicable" mean in the above description?
/**
- * Returns whether the result of the query execution is a collection of
rows, or not.
+ * Defines whether the result of a query is a collection of rows or not.
Review Comment:
I think defines is confusing, it provides information.
##########
modules/api/src/main/java/org/apache/ignite/Ignite.java:
##########
@@ -69,25 +68,19 @@ public interface Ignite extends AutoCloseable {
IgniteCompute compute();
/**
- * Returns {@link IgniteDeployment} which can be used to deploy units.
- *
- * @return Deployment management object.
- */
- IgniteDeployment deployment();
-
- /**
- * Gets the cluster nodes.
- * NOTE: Temporary API to enable Compute until we have proper Cluster API.
+ * Returns cluster nodes.
+ * NOTE: A temporary API to enable Compute until the permanent Cluster API
becomes available.
*
* @return Collection of cluster nodes.
*/
Collection<ClusterNode> clusterNodes();
/**
- * Gets the cluster nodes.
- * NOTE: Temporary API to enable Compute until we have proper Cluster API.
+ * Returns cluster nodes.
+ * NOTE: A temporary API to enable Compute until the permanent Cluster API
becomes available.
*
* @return Collection of cluster nodes.
*/
CompletableFuture<Collection<ClusterNode>> clusterNodesAsync();
+ //The same description is offered for the last two methods... shouldn't
there be a difference? One is "async".
Review Comment:
The only difference is that async method is Asynchronous. We should mention
it in the description.
##########
modules/api/src/main/java/org/apache/ignite/lang/UnexpectedNullValueException.java:
##########
@@ -17,31 +17,19 @@
package org.apache.ignite.lang;
-import java.util.UUID;
-
/**
- * This exception is thrown instead of returning a null value from a method
which doesn't respect {@code null}-value to avoid ambiguity
- * whether the value is absent or value is {@code null}.
+ * This exception is thrown instead of returning a null value from a method
that does not respect {@code null}-value to avoid ambiguity
+ * (whether the value is absent or is {@code null}).
*/
public class UnexpectedNullValueException extends IgniteException {
/**
- * Creates a new exception.
+ * Creates an exception.
*
* @param msg Message.
*/
public UnexpectedNullValueException(String msg) {
super("Got unexpected null value: " + msg);
}
-
- /**
- * Creates a new exception with the given trace id, error code, detail
message and cause.
- *
- * @param traceId Unique identifier of this exception.
- * @param code Full error code.
- * @param message Detail message.
- * @param cause Optional nested exception (can be {@code null}).
- */
- public UnexpectedNullValueException(UUID traceId, int code, String
message, Throwable cause) {
- super(traceId, code, message, cause);
- }
}
+
+//Creates an exception with the given message?
Review Comment:
I am not sure which class this refers to. If its
UnexpectedNullValueException then yes, its a way to avoid nullpointer
exceptions.
##########
modules/api/src/main/java/org/apache/ignite/table/Tuple.java:
##########
@@ -156,107 +156,108 @@ static boolean equals(Tuple firstTuple, Tuple
secondTuple) {
}
/**
- * Indicates whether some other object is "equal to" this one.
+ * Indicates whether another object is "equal to" the specified one.
*
- * @return {@code true} if this object is the same as the obj argument;
{@code false} otherwise.
+ * @return {@code true} if this object is the same as the specified one;
{@code false} otherwise.
* @see Tuple#equals(Tuple, Tuple)
* @see Object#equals(Object)
*/
boolean equals(Object obj);
+ //I don't see where the "current" object is specified (i.e., the obj param)
Review Comment:
I think it would be written as Object1.equals(Object2).
##########
modules/api/src/main/java/org/apache/ignite/binary/BinaryObject.java:
##########
@@ -23,9 +23,10 @@
//TODO: IGNITE-14316: Replace this stub with a proper interface.
public interface BinaryObject {
/**
- * Return binary object bytes.
+ * Returns binary object bytes.
*
* @return Serialized data.
*/
byte[] bytes();
}
+//Not clear what any of the Javadoc comments mean in this file.
Review Comment:
It returns data in bytes type - just literally the bytes of the file in
binary format.
##########
modules/api/src/main/java/org/apache/ignite/sql/async/AsyncResultSet.java:
##########
@@ -49,96 +46,97 @@
* }
* </code></pre>
*
- * @param <T> A type of the objects contained by this result set (when row set
is present). This will be either {@link SqlRow}
- * if no explicit mapper is provided or a particular type defined by
supplied mapper.
- *
* @see ResultSet
- * @see Session#executeAsync(Transaction, String, Object...)
- * @see Session#executeAsync(Transaction, Mapper, String, Object...)
*/
-public interface AsyncResultSet<T> {
+public interface AsyncResultSet {
/**
- * Returns metadata for the results if the result contains rows ({@link
#hasRowSet()} returns {@code true}), or {@code null} if
- * inapplicable.
+ * Returns metadata for query results. If the result set contains rows
({@link #hasRowSet()}, returns {@code true}).
+ * If not applicable, returns {@code null}.
*
- * @return ResultSet metadata.
+ * @return ResultSet Metadata.
* @see ResultSet#metadata()
*/
@Nullable ResultSetMetadata metadata();
+//What does "if not applicable" mean in the above description?
/**
- * Returns whether the result of the query execution is a collection of
rows, or not.
+ * Defines whether the result of a query is a collection of rows or not.
*
- * <p>Note: when returns {@code false}, then calling {@link
#currentPage()} will failed, and either {@link #affectedRows()} return
+ * <p>Note: If the method returns {@code false}, calling {@link
#currentPage()} will fail, and either {@link #affectedRows()} return
* number of affected rows or {@link #wasApplied()} returns {@code true}.
*
- * @return {@code True} if the query returns rows, {@code false} otherwise.
+ * @return {@code True} if a query returned rows, {@code false} otherwise.
*/
boolean hasRowSet();
+//I don't understand what the Note above means.
/**
- * Returns number of rows affected by the DML statement execution (such as
"INSERT", "UPDATE", etc.), or {@code 0} if statement return
- * nothing (such as "ALTER TABLE", etc) or {@code -1} if inapplicable.
+ * Returns the number of rows affected by the DML statement execution
(such as "INSERT", "UPDATE", etc.), or {@code 0} if the statement returns
+ * nothing (such as "ALTER TABLE", etc), or {@code -1} if inapplicable.
*
- * <p>Note: when returns {@code -1}, then either {@link #hasRowSet()} or
{@link #wasApplied()} returns {@code true}.
+ * <p>Note: If the method returns {@code -1}, either {@link #hasRowSet()}
or {@link #wasApplied()} returns {@code true}.
*
- * @return Number of rows affected by the query, or {@code 0} if statement
return nothing, or {@code -1} if inapplicable.
+ * @return Number of rows affected by the query, or {@code 0} if the
statement returns nothing, or {@code -1} if not applicable.
* @see ResultSet#affectedRows()
*/
long affectedRows();
+//What does "not applicable" above mean?
+
/**
- * Returns whether the query that produce this result was a conditional
query, or not. E.g. for the query "Create table if not exists"
- * the method returns {@code true} when an operation was applied
successfully, and {@code false} when an operation was ignored due to
- * table was already existed.
+ * Indicates whether the query that had produced the result was a
conditional query.
+ * E.g., for query "Create table if not exists", the method returns {@code
true} if
+ * the operation was successful or {@code false} if the operation was
ignored because the table already existed.
*
- * <p>Note: when returns {@code false}, then either {@link
#affectedRows()} return number of affected rows or {@link #hasRowSet()}
- * returns {@code true} or conditional DDL query is not applied.
+ * <p>Note: If the method returns {@code false}, then either {@link
#affectedRows()} returns the number of
+ * affected rows, or {@link #hasRowSet()} returns {@code true}, or the
conditional DDL query is not applied.
*
- * @return {@code True} if conditional query applied, {@code false}
otherwise.
+ * @return {@code True} if a conditional query is applied, {@code false}
otherwise.
* @see ResultSet#wasApplied()
*/
boolean wasApplied();
/**
- * Returns the current page content if the query return rows.
+ * Returns the current page content if the query returns rows.
*
- * @return Iterable over rows.
- * @throws NoRowSetExpectedException if no row set is expected as a query
result.
+ * @return Iterable set of rows.
+ * @throws NoRowSetExpectedException if no row set is returned.
*/
- Iterable<T> currentPage();
+ Iterable<SqlRow> currentPage();
/**
* Returns the current page size if the query return rows.
*
* @return The size of {@link #currentPage()}.
- * @throws NoRowSetExpectedException if no row set is expected as a query
result.
+ * @throws NoRowSetExpectedException if no row set is returned.
*/
int currentPageSize();
/**
- * Fetch the next page of results asynchronously.
+ * Fetches the next page of results asynchronously.
* The future that is completed with the same {@code AsyncResultSet}
object.
- * The current page is changed after the future complete.
+ * The current page is changed after the future completion.
* The methods {@link #currentPage()}, {@link #currentPageSize()}, {@link
#hasMorePages()}
- * use current page and return consistent results between complete last
page future and call {@code fetchNextPage}.
+ * use the current page and return consistent results between complete
last page future and call {@code fetchNextPage}.
*
* @return Operation future.
* @throws NoRowSetExpectedException if no row set is expected as a query
result.
*/
- CompletableFuture<? extends AsyncResultSet<T>> fetchNextPage();
+ CompletionStage<? extends AsyncResultSet> fetchNextPage();
+
+//I cannot deduce what writer tried to say about this method.
Review Comment:
The method works asynchronously.
The result of the method completing the future is the same AsyncResultSet
object.
The page the mehtod will work with in the database is changed after the
method is executed.
Methods {} will always return consistent results, even if this method is
running - this is done to avoid possible races.
##########
modules/api/src/main/java/org/apache/ignite/sql/async/AsyncResultSet.java:
##########
@@ -49,96 +46,97 @@
* }
* </code></pre>
*
- * @param <T> A type of the objects contained by this result set (when row set
is present). This will be either {@link SqlRow}
- * if no explicit mapper is provided or a particular type defined by
supplied mapper.
- *
* @see ResultSet
- * @see Session#executeAsync(Transaction, String, Object...)
- * @see Session#executeAsync(Transaction, Mapper, String, Object...)
*/
-public interface AsyncResultSet<T> {
+public interface AsyncResultSet {
/**
- * Returns metadata for the results if the result contains rows ({@link
#hasRowSet()} returns {@code true}), or {@code null} if
- * inapplicable.
+ * Returns metadata for query results. If the result set contains rows
({@link #hasRowSet()}, returns {@code true}).
+ * If not applicable, returns {@code null}.
*
- * @return ResultSet metadata.
+ * @return ResultSet Metadata.
* @see ResultSet#metadata()
*/
@Nullable ResultSetMetadata metadata();
+//What does "if not applicable" mean in the above description?
/**
- * Returns whether the result of the query execution is a collection of
rows, or not.
+ * Defines whether the result of a query is a collection of rows or not.
*
- * <p>Note: when returns {@code false}, then calling {@link
#currentPage()} will failed, and either {@link #affectedRows()} return
+ * <p>Note: If the method returns {@code false}, calling {@link
#currentPage()} will fail, and either {@link #affectedRows()} return
* number of affected rows or {@link #wasApplied()} returns {@code true}.
*
- * @return {@code True} if the query returns rows, {@code false} otherwise.
+ * @return {@code True} if a query returned rows, {@code false} otherwise.
*/
boolean hasRowSet();
+//I don't understand what the Note above means.
Review Comment:
If the method returns false, currentPage method will fail and should not be
used. However, either affectedRows or wasApplied method will return a valid
answer, so you should use these instead.
##########
modules/api/src/main/java/org/apache/ignite/sql/async/AsyncResultSet.java:
##########
@@ -49,96 +46,97 @@
* }
* </code></pre>
*
- * @param <T> A type of the objects contained by this result set (when row set
is present). This will be either {@link SqlRow}
- * if no explicit mapper is provided or a particular type defined by
supplied mapper.
- *
* @see ResultSet
- * @see Session#executeAsync(Transaction, String, Object...)
- * @see Session#executeAsync(Transaction, Mapper, String, Object...)
*/
-public interface AsyncResultSet<T> {
+public interface AsyncResultSet {
/**
- * Returns metadata for the results if the result contains rows ({@link
#hasRowSet()} returns {@code true}), or {@code null} if
- * inapplicable.
+ * Returns metadata for query results. If the result set contains rows
({@link #hasRowSet()}, returns {@code true}).
+ * If not applicable, returns {@code null}.
*
- * @return ResultSet metadata.
+ * @return ResultSet Metadata.
* @see ResultSet#metadata()
*/
@Nullable ResultSetMetadata metadata();
+//What does "if not applicable" mean in the above description?
/**
- * Returns whether the result of the query execution is a collection of
rows, or not.
+ * Defines whether the result of a query is a collection of rows or not.
*
- * <p>Note: when returns {@code false}, then calling {@link
#currentPage()} will failed, and either {@link #affectedRows()} return
+ * <p>Note: If the method returns {@code false}, calling {@link
#currentPage()} will fail, and either {@link #affectedRows()} return
* number of affected rows or {@link #wasApplied()} returns {@code true}.
*
- * @return {@code True} if the query returns rows, {@code false} otherwise.
+ * @return {@code True} if a query returned rows, {@code false} otherwise.
*/
boolean hasRowSet();
+//I don't understand what the Note above means.
/**
- * Returns number of rows affected by the DML statement execution (such as
"INSERT", "UPDATE", etc.), or {@code 0} if statement return
- * nothing (such as "ALTER TABLE", etc) or {@code -1} if inapplicable.
+ * Returns the number of rows affected by the DML statement execution
(such as "INSERT", "UPDATE", etc.), or {@code 0} if the statement returns
+ * nothing (such as "ALTER TABLE", etc), or {@code -1} if inapplicable.
*
- * <p>Note: when returns {@code -1}, then either {@link #hasRowSet()} or
{@link #wasApplied()} returns {@code true}.
+ * <p>Note: If the method returns {@code -1}, either {@link #hasRowSet()}
or {@link #wasApplied()} returns {@code true}.
*
- * @return Number of rows affected by the query, or {@code 0} if statement
return nothing, or {@code -1} if inapplicable.
+ * @return Number of rows affected by the query, or {@code 0} if the
statement returns nothing, or {@code -1} if not applicable.
* @see ResultSet#affectedRows()
*/
long affectedRows();
+//What does "not applicable" above mean?
Review Comment:
we should be able to change all of these to otherwise.
##########
modules/api/src/main/java/org/apache/ignite/lang/MarshallerException.java:
##########
@@ -17,31 +17,19 @@
package org.apache.ignite.lang;
-import java.util.UUID;
-
/**
- * Exception is thrown when failed to marshall or unmarshall value.
- * E.g. due to a value mismatch a schema or any other reason.
+ * This exception is caused by a failure to marshall or unmarshall a value.
+ * The failure can be due to a value not matching the a schema or to another
reason.
*/
public class MarshallerException extends IgniteException {
/**
- * Creates a new exception with the given error message.
+ * Creates an exception with the given error message.
*
* @param cause Non-null throwable cause.
*/
public MarshallerException(Throwable cause) {
super(cause);
}
-
- /**
- * Creates a new exception with the given trace id, error code, detail
message and cause.
- *
- * @param traceId Unique identifier of this exception.
- * @param code Full error code.
- * @param message Detail message.
- * @param cause Optional nested exception (can be {@code null}).
- */
- public MarshallerException(UUID traceId, int code, String message,
Throwable cause) {
- super(traceId, code, message, cause);
- }
}
+
+//The description does not seem to match the parameter.
Review Comment:
Parameter is the Throwable object, a generic java thing for errors.
--
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]
