Skip to content
This repository has been archived by the owner on Jul 27, 2021. It is now read-only.

refactor: CI, integration tests, cleanup #8

Merged
merged 3 commits into from
Jun 9, 2021
Merged

Conversation

cristianmtr
Copy link
Contributor

@cristianmtr cristianmtr commented Jun 7, 2021

@cristianmtr cristianmtr force-pushed the feat-psql-20 branch 3 times, most recently from 9f9ad47 to 8f3d108 Compare June 7, 2021 12:23
@cristianmtr cristianmtr force-pushed the feat-psql-20 branch 2 times, most recently from df6b97b to 460acc5 Compare June 7, 2021 15:32
@cristianmtr cristianmtr marked this pull request as draft June 7, 2021 15:32
@cristianmtr cristianmtr marked this pull request as ready for review June 8, 2021 10:55
Copy link
Member

@maximilianwerk maximilianwerk left a comment

Choose a reason for hiding this comment

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

Will have another look later. Looks really good.

jinahub/indexers/dump/dump.py Outdated Show resolved Hide resolved
@cristianmtr cristianmtr force-pushed the feat-psql-20 branch 14 times, most recently from 2c6f4b7 to 8cfb89a Compare June 9, 2021 07:33
Copy link
Member

@maximilianwerk maximilianwerk left a comment

Choose a reason for hiding this comment

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

Some remarks.

jinahub/indexers/dbms/PostgreSQLIndexer/__init__.py Outdated Show resolved Hide resolved
password=self.password,
database=self.database,
table=self.table)

def get_handler(self) -> 'PostgreSQLDBMSHandler':
Copy link
Member

Choose a reason for hiding this comment

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

These get_XXX_handler functions are a smell to me. Just use self.handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think those were necessary because of the BaseIndexer's close call, which would call these. Removed

@@ -58,22 +69,12 @@ def size(self):
.. # noqa: DAR201
"""
with self.handler as postgres_handler:
Copy link
Member

Choose a reason for hiding this comment

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

I am not convinced of opening/closing connection with every operation. This creates quite some overhead, especially when the database is running on a distance machine. Why not open the connection once and keep it open?

jinahub/indexers/dbms/PostgreSQLIndexer/postgreshandler.py Outdated Show resolved Hide resolved
jinahub/indexers/query/vector/NumpyIndexer/__init__.py Outdated Show resolved Hide resolved
jinahub/indexers/query/vector/NumpyIndexer/__init__.py Outdated Show resolved Hide resolved
@cristianmtr cristianmtr changed the title feat: psql 2.0 refactor: CI, integration tests, cleanup Jun 9, 2021
Copy link
Member

@maximilianwerk maximilianwerk left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants