Skip to content

Conversation

@zhenlineo
Copy link
Contributor

Used ChannelGroup to keep track of all alive channels.

Used ChannelGroup to keep track of all alive channels.
@zhenlineo zhenlineo requested a review from lutovich September 19, 2018 08:47
Copy link
Contributor

@lutovich lutovich left a comment

Choose a reason for hiding this comment

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

@zhenlineo review round completed. Made couple comments.

MetricsListener metricsListener, Logging logging, Clock clock )
{
this( connector, bootstrap, new NettyChannelTracker( metricsListener, logging ), settings, metricsListener, logging, clock );
this( connector, bootstrap, new NettyChannelTracker( metricsListener, eventExecutorGroup, logging ), settings, metricsListener, logging, clock );
Copy link
Contributor

Choose a reason for hiding this comment

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

I think EventExecutorGroup parameter can be removed. Channel tracker can be created using the given bootstrap:

new NettyChannelTracker( metricsListener, bootstrap.config().group().next(), logging )

ChannelPool pool = entry.getValue();

log.info( "Closing connection pool towards %s", address );
nettyChannelTracker.destructAllChannels();
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a single channel tracker for all pools. This method is invoked once for every pool. Shouldn't it be called once outside of the outer loop?

decrementIdle( channel );
metricsListener.afterClosed( serverAddress( channel ) );

allChannels.remove( channel );
Copy link
Contributor

Choose a reason for hiding this comment

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

Closed channels are automatically removed from a channel group. This line can be removed

* Destruct channel before it is destroyed.
* @param channel the channel to destroy.
*/
void destructChannel( Channel channel );
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe #prepareToClose()?

RECORD [1]
SUCCESS {"bookmark": "bookmark:1"}
C: GOODBYE
S: SUCCESS {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Database does not send SUCCESS for GOODBYE. Better replace with S: <EXIT>

Copy link
Contributor

@lutovich lutovich left a comment

Choose a reason for hiding this comment

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

@zhenlineo two small comments. Looks good.

private final ChannelGroup allChannels;

public NettyChannelTracker( MetricsListener metricsListener, Logging logging )
public NettyChannelTracker( MetricsListener metricsListener, EventExecutorGroup eventExecutorGroup, Logging logging )
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can just take and EventExecutor and pass it directly to the DefaultChannelGroup constructor. bootstrap.config().group().next() returns an EventExecutor.

catch ( Throwable e )
{
// only logging it
log.debug( "Failed to destruct Channel %s due to error %s. " +
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe reword this a bit to remove "destruct"?

@lutovich lutovich merged commit 0a4eb49 into neo4j:1.7 Sep 24, 2018
@zhenlineo zhenlineo deleted the 1.7-goodbye branch September 24, 2018 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants