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

Remove my postgres connection pools #149

Merged
merged 4 commits into from
Feb 3, 2017

Conversation

iandees
Copy link
Member

@iandees iandees commented Jan 31, 2017

Backs out the connection pool bits of #145 (because they didn't work in production), puts back the existing DBAffinityConnectionsNoLimit implementation and adds the one-liner to use ujson.loads for register_json().

self.sql_conn_pool = DatabaseCycleConnectionPool(
n_conn, max_conn, self.dbnames, self.conn_info)
self.sql_conn_pool = DBAffinityConnectionsNoLimit(
self.dbnames, self.conn_info)
Copy link
Member

Choose a reason for hiding this comment

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

This section around line 160 used to read:

        self.sql_conn_pool = DBAffinityConnectionsNoLimit(
            self.dbnames, n_conn, self.conn_info)

Where self.n_conn = n_conn was part of the function arguments.

Similarly on 170 we used to say:

        sql_conns, conn_info = self.sql_conn_pool.get_conns()

instead of proposed:

         sql_conns = self.sql_conn_pool.get_conns(self.n_conn)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that was fairly messy, so this is a cleanup of that.

import ujson


class DBAffinityConnectionsNoLimit(object):
Copy link
Member

Choose a reason for hiding this comment

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

Looking back at history I was expecting to see 3x as much content here, mostly based off this SHA:

And then bringing some of the hstore / ujson changes in from:

Copy link
Member Author

Choose a reason for hiding this comment

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

We didn't use the other database pools, so I removed them.

The ujson stuff is there. The globally=True thing I wanted to leave off in case it caused the connection problem.

@@ -533,7 +533,7 @@ def tilequeue_process(cfg, peripherals):
io_pool = ThreadPool(n_io_workers)

feature_fetcher = DataFetcher(cfg.postgresql_conn_info, all_layer_data,
io_pool, n_layers, n_total_needed_query)
io_pool, n_layers)
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@nvkelso nvkelso requested a review from zerebubuth February 1, 2017 00:54
@nvkelso
Copy link
Member

nvkelso commented Feb 1, 2017

@zerebubuth for more substantive comments.

@nvkelso
Copy link
Member

nvkelso commented Feb 3, 2017

@iandees Can you cleanup the merge conflict, please?

@iandees
Copy link
Member Author

iandees commented Feb 3, 2017

🤔 I did, but the commit's not showing up ... I re-pushed it just now and I see it showed up.

@iandees
Copy link
Member Author

iandees commented Feb 3, 2017

I'm going to go ahead and merge this per the tiles catchup meeting today.

@iandees iandees merged commit 1c85100 into master Feb 3, 2017
@iandees iandees deleted the remove_my_postgres_connection_pools branch February 3, 2017 19:51
@iandees iandees removed the in review label Feb 3, 2017
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.

3 participants