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

resolve catalog properties on the error path. #11831

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@hgschmie
Contributor

hgschmie commented Oct 31, 2018

This fixes issue #11830.

else {
transactionId = transactionManager.beginTransaction(false);
// resolve the catalog session properties in the error path.
session = session.beginTransactionId(transactionId, transactionManager, accessControl);

This comment has been minimized.

@dain

dain Nov 1, 2018

Contributor

I'm concerned about this change. I believe this call can also fail, which would cause an uncaught error in the current code. Also, with the Dispatcher change coming soon, the dispatcher won't even have connectors registered so, there is no way to "resolve" connector sessions properties during this phase of the execution.

This comment has been minimized.

@hgschmie

hgschmie Nov 1, 2018

Contributor

So what is the right thing to do here? There are some session properties that are specific to a catalog (they come from the connector but are resolved in catalog scope) and without those e.g. our event listeners do not work.

Is there another way besides a transaction to resolve those?

This comment has been minimized.

@hgschmie

hgschmie Nov 5, 2018

Contributor

@dain mentioned that he has a bigger patch that would affect that part of the code. I still would like to revisit this problem after those changes go in because they affect consumers of catalog properties.

@dain

This comment has been minimized.

Contributor

dain commented Nov 13, 2018

Resolved by #11831

@dain dain closed this Nov 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment