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 transaction when listing projects #522

Conversation

davidheryanto
Copy link
Collaborator

@davidheryanto davidheryanto commented Mar 9, 2020

What this PR does / why we need it:
Listing projects is a read only operation and therefore should not need the @Transactional Hibernate annotation.

Having @Transactional annotation results in excessive locks being used when listing projects because of the INSERT and DELETE queries generated. As a result, when other operations such as job updates (which is also Transactional) is running, users trying to list projects will often encounter GRPC error TimeoutExceeded because ListProjects will wait a long for the shared lock to be available.

When there is at least one feature set registered, these are the generated queries when listing projects:

sql.with.transaction.log
sql.no.transaction.log

Without @Transaction no INSERT and DELETE queries are generated and hence it will not wait for lock to be available when listing projects.

The generated queries can be logged with this log4j2 configuration in feast-core:

<Logger name="org.hibernate.SQL" level="debug" additivity="false">
      <AppenderRef ref="ConsoleAppender"/>
</Logger>

Which issue(s) this PR fixes:

NONE

Does this PR introduce a user-facing change?:

NONE

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidheryanto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zhilingc
Copy link
Collaborator

zhilingc commented Mar 9, 2020

IMO none of these methods need to be transactional, but
/lgtm

@feast-ci-bot feast-ci-bot merged commit aa9b1a5 into feast-dev:master Mar 9, 2020
khorshuheng pushed a commit to khorshuheng/feast that referenced this pull request Mar 16, 2020
khorshuheng pushed a commit to khorshuheng/feast that referenced this pull request Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants