Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,28 +14,114 @@
import io.tarantool.core.protocol.IProtoMessage;
import io.tarantool.core.protocol.IProtoResponse;

/** An interface representing interaction with a Tarantool node via a connection abstraction. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** An interface representing interaction with a Tarantool node via a connection abstraction. */
/** A single connection to a Tarantool node. */

public interface Connection extends AutoCloseable {

/**
* Method for asynchronous connection to a Tarantool node.
*
* @param addr address at which the connection is being established
* @param timeoutMs timeout, specified in milliseconds, that defines the maximum wait time between
* the start of a connection attempt and the receipt of a greeting packet
* @return an instance of {@link CompletableFuture} that completes with a greeting or an error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major. Не описан IllegalStateException при timeoutMs <= 0: ConnectionImpl#scheduleGreetingTimeout бросает IllegalStateException("Timeout must be greater than zero!"). Кроме того, ConnectionImpl#connect в состоянии CONNECTING/WAITING/READY молча возвращает прежний promise — контракт об этом умалчивает. Предлагаю привести реализацию к контракту:

    } else if (state.get() != State.CLOSED) {
      throw new IllegalStateException(state.get().getMessage());
    }

и описать это в @throws.

*/
Comment on lines +20 to +27

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Method for asynchronous connection to a Tarantool node.
*
* @param addr address at which the connection is being established
* @param timeoutMs timeout, specified in milliseconds, that defines the maximum wait time between
* the start of a connection attempt and the receipt of a greeting packet
* @return an instance of {@link CompletableFuture} that completes with a greeting or an error.
*/
/**
* Asynchronously connects to a Tarantool node.
*
* @param addr address to connect to
* @param timeoutMs maximum time in milliseconds to wait for the greeting packet
* @return a future completing with the greeting packet or completing exceptionally on failure
*/

CompletableFuture<Greeting> connect(InetSocketAddress addr, long timeoutMs)
throws IllegalStateException;

/**
* Asynchronously sends an {@link IProtoMessage} object to a Tarantool node.
*
* @param message the message to be sent to Tarantool.
* @return a {@link CompletableFuture} object.
Comment thread
bitgorbovsky marked this conversation as resolved.
* @throws IllegalStateException if the method is called when the connection is not in a state
* ready to send or receive messages.
*/
Comment on lines +31 to +38

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Asynchronously sends an {@link IProtoMessage} object to a Tarantool node.
*
* @param message the message to be sent to Tarantool.
* @return a {@link CompletableFuture} object.
* @throws IllegalStateException if the method is called when the connection is not in a state
* ready to send or receive messages.
*/
/**
* Asynchronously sends an {@link IProtoMessage} to a Tarantool node.
*
* @param message message to send
* @return a future completing when the message has been written to the channel
* @throws IllegalStateException if the connection is not ready to send messages
*/

CompletableFuture<Void> send(IProtoMessage message) throws IllegalStateException;

/**
* Registers a callback that will receive any {@link IProtoResponse} message arriving from a
* Tarantool node.
*
* @param listener A handler for responses from Tarantool. Must not be {@code null}.
* @return The same connection object on which this method was called.
* @implSpec Each subsequent call to this method will overwrite the previously registered handler.
* Not thread-safe.
* @apiNote Typically used when connection object are creating and not used thereafter.
*/
Comment on lines +41 to +50

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Registers a callback that will receive any {@link IProtoResponse} message arriving from a
* Tarantool node.
*
* @param listener A handler for responses from Tarantool. Must not be {@code null}.
* @return The same connection object on which this method was called.
* @implSpec Each subsequent call to this method will overwrite the previously registered handler.
* Not thread-safe.
* @apiNote Typically used when connection object are creating and not used thereafter.
*/
/**
* Registers a listener for responses arriving from the Tarantool node.
*
* @param listener listener receiving every {@link IProtoResponse}
* @return this connection
* @apiNote Each call replaces the previously registered listener. Registration is not
* thread-safe and is typically done once, when the connection object is being created.
*/

Connection listen(Consumer<IProtoResponse> listener);

/**
* A method that registers a callback to handle a connection-close event.
*
* @param event the connection-close event ({@link ConnectionCloseEvent}) for which the handler is
* being registered.
* @param handler the handler object.
* @implSpec If multiple handlers are provided for the same event, the implementation must
Comment thread
bitgorbovsky marked this conversation as resolved.
* register all of them, rather than replacing a previously registered handler with a new one.
Comment thread
bitgorbovsky marked this conversation as resolved.
* All callbacks are guaranteed to be invoked after the connection has transitioned to the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. Утверждение не соответствует реализации. В ConnectionImpl#onChannelClose уведомление выполняется до смены состояния и обнуления канала:

    if (state.get() != State.CONNECTING) {
      Exception ex = new ConnectionClosedException(message, f.cause());
      failConnectPromise(ex);
      fire(event, ex);          // состояние CLOSING / READY, channel ещё не null
    }
    state.set(State.CLOSED);
    channel = null;

Для close()/shutdownClose() обработчик видит CLOSING, для удалённого закрытия — READY/WAITING; channel ещё доступен, поэтому, например, isPaused() внутри обработчика отработает, хотя после закрытия он обязан падать.

Предлагаю убрать это предложение. Если гарантия нужна, сначала завершить переход, затем уведомлять:

    boolean notify = state.get() != State.CONNECTING;
    Exception ex = notify ? new ConnectionClosedException(message, f.cause()) : null;
    if (notify) {
      failConnectPromise(ex);
    }
    state.set(State.CLOSED);
    channel = null;
    connectPromise = null;
    if (notify) {
      fire(event, ex);
    }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вероятно, что-то из этого правда

* closed state.
* @apiNote Typically used during the creation of a connection object and not used thereafter.
*/
Comment on lines +53 to +64

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* A method that registers a callback to handle a connection-close event.
*
* @param event the connection-close event ({@link ConnectionCloseEvent}) for which the handler is
* being registered.
* @param handler the handler object.
* @implSpec If multiple handlers are provided for the same event, the implementation must
* register all of them, rather than replacing a previously registered handler with a new one.
* All callbacks are guaranteed to be invoked after the connection has transitioned to the
* closed state.
* @apiNote Typically used during the creation of a connection object and not used thereafter.
*/
/**
* Registers a handler for a connection-close event.
*
* <p>All handlers registered for the same event are invoked; a new handler does not replace
* previously registered ones.
*
* @param event close event to subscribe to
* @param handler handler receiving this connection and the close cause
* @return this connection
* @apiNote Registration is not thread-safe and is typically done once, when the connection
* object is being created.
*/

Connection onClose(ConnectionCloseEvent event, BiConsumer<Connection, Throwable> handler);

/**
* Gets greeting packet.
*
* @return {@link Optional#empty()} if this method was called before the connection reached a
Comment thread
bitgorbovsky marked this conversation as resolved.
* state ready.
*/
Comment thread
bitgorbovsky marked this conversation as resolved.
Optional<Greeting> getGreeting();

/**
* Checks whether the connection is currently active and ready to process requests.
*
* <p>Note that this status represents a snapshot in time. The connection state may change
* immediately after this method returns.
*
* @return {@code true} if the connection is currently in the ready state; {@code false} otherwise
*/
boolean isConnected();

/**
* Closes the connection reporting the close cause as a graceful shutdown of the server.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Closes the connection reporting the close cause as a graceful shutdown of the server.
* Closes the connection because the node initiated a graceful shutdown.

*
* <p>The method is meant to be called upon receiving the {@code box.shutdown} notification from
* Tarantool. At that moment the connection is still operational, it is the server that is about
* to terminate.
*
* <p>In contrast to {@link #close()}, the close is reported to the listeners as {@link
Comment thread
bitgorbovsky marked this conversation as resolved.
* ConnectionCloseEvent#CLOSE_BY_SHUTDOWN} rather than {@link
* ConnectionCloseEvent#CLOSE_BY_CLIENT}, and they are expected to react differently: a connection
* closed by the client needs no attention, while a connection closed by shutdown has to be
* replaced with a new one. The method itself does not reestablish the connection.
*
* <p>Requests pending on the connection are not failed by this method, the caller is responsible
Comment thread
bitgorbovsky marked this conversation as resolved.
* for cancelling them. The method does nothing if the connection is already closed or is being
* closed.
*
* @see <a
* href="https://www.tarantool.io/ru/doc/latest/reference/internals/iproto/graceful_shutdown/">Tarantool
* graceful shutdown</a>
*/
void shutdownClose();

void setIdleTimeout(int idleTimeout);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. У метода нет javadoc. Нужно описать: единицы измерения — миллисекунды; > 0 включает проверку, <= 0 выключает; значение применяется при построении pipeline в ConnectionImpl#getInitializer, то есть только при следующем подключении.

  /**
   * Sets the idle timeout for the connection.
   *
   * <p>The value is applied when the connection is established, so it affects only the next
   * connection attempt.
   *
   * @param idleTimeout idle timeout in milliseconds; a positive value enables the check, a
   *     non-positive value disables it
   */
  void setIdleTimeout(int idleTimeout);


/**
* Method that pauses the reading of the buffer containing incoming data at the OS level for
Comment thread
bitgorbovsky marked this conversation as resolved.
* Netty.
*/
Comment on lines +110 to +113

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Method that pauses the reading of the buffer containing incoming data at the OS level for
* Netty.
*/
/** Stops reading incoming data from the socket. */

void pause();

/**
* Method that resumes the reading of the buffer containing incoming data at the OS level for
* Netty.
*/
Comment on lines +116 to +119

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Method that resumes the reading of the buffer containing incoming data at the OS level for
* Netty.
*/
/** Resumes reading incoming data previously stopped by {@link #pause()}. */

void resume();

void setIdleTimeout(int idleTimeout);

/**
* A method that checks whether automatic reading of a buffer with incoming data has been stopped
* at the OS level for Netty.
*/
Comment on lines +122 to +125

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* A method that checks whether automatic reading of a buffer with incoming data has been stopped
* at the OS level for Netty.
*/
/** Returns whether reading of incoming data is currently paused. */

boolean isPaused();
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,33 @@

package io.tarantool.core.connection;

/** {@link Connection} close events. */
public enum ConnectionCloseEvent {

/**
* This event reflects a situation where the connection is closed from the network side—for
Comment thread
bitgorbovsky marked this conversation as resolved.
* example, due to a network disconnection.
*/
Comment thread
bitgorbovsky marked this conversation as resolved.
CLOSE_BY_REMOTE,

/**
* An event reflecting a situation where the connection has been closed by the client-side calling
* code.
*/
Comment thread
bitgorbovsky marked this conversation as resolved.
CLOSE_BY_CLIENT,

/**
* This event reflects a situation where a connection is closed from the Tarantool side via a call
* to {@code box.shutdown}. Unlike {@link #CLOSE_BY_REMOTE}, this event does not indicate network
* issues; rather, it signifies that a message has been received from a Tarantool node initiating
* a graceful shutdown.
Comment thread
bitgorbovsky marked this conversation as resolved.
*
* @see <a
* href="https://www.tarantool.io/ru/doc/latest/reference/internals/iproto/graceful_shutdown/">Tarantool
* documentation</a>
*/
CLOSE_BY_SHUTDOWN,

/** An event representing a situation where a connection has been closed due to a timeout */
Comment thread
bitgorbovsky marked this conversation as resolved.
Comment thread
bitgorbovsky marked this conversation as resolved.
CLOSE_BY_TIMEOUT;
}
Loading