Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client service uses all ports #74

Closed
rado-r opened this issue Dec 3, 2019 · 12 comments
Closed

Client service uses all ports #74

rado-r opened this issue Dec 3, 2019 · 12 comments
Assignees
Labels
bug Something isn't working ready to test

Comments

@rado-r
Copy link

rado-r commented Dec 3, 2019

Hi,
We are testing ClickHouse with your connector and you did a great job with it. But we have a problem. When we run service in AWS as docker and try to insert a lot of records (2 billion) I don't know what is happening but connector gradually uses all ports in docker and never release it.

I try to close every connection in try catch and get a new connection for every batch insert. And I try to get new connection only if it was closed it was better because service uses fewer ports but still it was thousands of ports.

...
           try( final Connection connection = dataSource.getConnection();
            final PreparedStatement preparedStatement = connection.prepareStatement(
                    ActivitySqlGenerator.createInsertPreparedStatement(getDbAndTableName(), QuoteType.BACK_QUOTE)));
{
         connection.setAutoCommit(false);

        for (Activity activity: activities) {
            ActivitySqlGenerator.bindInsert(1, preparedStatement, activity);
            preparedStatement.addBatch();
        }

        preparedStatement.executeBatch();
        preparedStatement.clearBatch();
        connection.commit();

        } catch (Exception e) {
            logger.info("Failed to bulk insert.", e);
        }
...
    public ClickHouseConnection getConnection() throws SQLException {
        if (connection == null || connection.isClosed()) {
            this.connection = dataSource.getConnection();
        }
        return this.connection;
    }

...
        try
        {
            final Connection connection = this.getConnection();
            final PreparedStatement preparedStatement = connection.prepareStatement(
                    ActivitySqlGenerator.createInsertPreparedStatement(getDbAndTableName(), QuoteType.BACK_QUOTE));

        connection.setAutoCommit(false);

        for (Activity activity: activities) {
            ActivitySqlGenerator.bindInsert(1, preparedStatement, activity);
            preparedStatement.addBatch();
        }

        preparedStatement.executeBatch();
        preparedStatement.clearBatch();
        connection.commit();

        } catch (Exception e) {
            logger.info("Failed to bulk insert.", e);
        }
...

Can someone help? Is It known problem or do I something wrong?

@doom369
Copy link
Collaborator

doom369 commented Dec 3, 2019

Hello. Code in the first snippet looks correct. From what I understand - you don't use Connection Pool. Is that correct? What driver version do you use?

Also, second snippet doesn't close the connections. While first one - does.

@doom369 doom369 self-assigned this Dec 3, 2019
@rado-r
Copy link
Author

rado-r commented Dec 3, 2019

Yes first one close connection every batch insert in second one I try to reuse connections if they are not closed. Yes I don't use Connection Pool and I try clickhouse4j 1.1.1 and 1.4.1 with same results.

@doom369
Copy link
Collaborator

doom369 commented Dec 3, 2019

If you want to reuse the connection I would recommend you to use the Connection pool. Because connections are not threadsafe and managing multiple connections would be a tricky task. While connection pools keeps them keep-alive, managing them and makes access to them threadsafe.

Also, few comments regarding you code:

You don't need to make connection and prepared statement as final if you use them in try () catch, they are effectively final anyway.
You don't need to preparedStatement.clearBatch() in case preparedStatement was in try (preparedStatement) block as it will be closed and cleared by GC anyway.

@doom369
Copy link
Collaborator

doom369 commented Dec 3, 2019

@rado-r do you use the code without try () catch in multi threaded env?

@doom369 doom369 added bug Something isn't working in progress labels Dec 3, 2019
@doom369
Copy link
Collaborator

doom369 commented Dec 3, 2019

@doom369
Copy link
Collaborator

doom369 commented Dec 4, 2019

@rado-r hello. do you have any updates? Also, what Java version do you have?

@rado-r
Copy link
Author

rado-r commented Dec 4, 2019

@doom369 thank's for very fast response I test it and now it is fixed. I test old released version with connection pool (Hikari) but it don't releases ports too but it was only hundreds of ports. After this fix it is 0 ports. So gread job.

We use Java 8.

@doom369
Copy link
Collaborator

doom369 commented Dec 4, 2019

@rado-r so you used the provided build and it is fixes the issue, is that correct?

How many open connections did you saw? Were they in CLOSE_WAIT state?

@rado-r
Copy link
Author

rado-r commented Dec 4, 2019

@doom369 yes provided build fix the issue.
Before fix I try to insert 2 billion records and connector uses all ports of docker there wasn't any free port to continue.

netstat -pnt |grep CLOSE_WAIT| wc -l
28232

@rado-r
Copy link
Author

rado-r commented Dec 4, 2019

What do you think when it will be on maven?

@doom369
Copy link
Collaborator

doom369 commented Dec 4, 2019

We will try to release today. However, this situation is very strange. Because in this driver we rely on HTTPUrlConnection (while original driver relies on apache commons connection pool) in which connection pool is handled on the level of JVM. In old JVM there was a bug with non-closed connections. However, it was fixed many days ago, so now it should work. Also, on our envs (we have dozens of them) it is not the case. So it would be cool to find the actual cause.

Could you please provide also exact version of JVM and OS?

@rado-r
Copy link
Author

rado-r commented Dec 4, 2019

It should be

OpenJDK Runtime Environment (build 1.8.0_232-b09)
OpenJDK 64-Bit Server VM (build 25.232-b09, mixed mode)

in docker openjdk:8-jre but it is customised docker image so I'm not 100% sure.

doom369 added a commit that referenced this issue Dec 4, 2019
* #74 fix for connections leak when no Connection Pool used

* #74 fix for connections leak when no Connection Pool used, null check
@doom369 doom369 closed this as completed Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready to test
Projects
None yet
Development

No branches or pull requests

2 participants