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

cqlsh.py: fix server side describe after login command #89

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

fruch
Copy link
Collaborator

@fruch fruch commented May 30, 2024

since login command create a new session, we run into an issue when describe was called after login
seem like the row_factory wasn't used

the reason was in the main session we used execution_profiles while in the one create in login we did not, which led to copying the wrong values (i.e. like row_factory that was needed for server side describe)

this change saves the profiles into the instance, and reuse them when ever a new session is opened
it simplify the code and we could remove a few repetitions of the same logic

since login command create a new session, we run into an
issue when describe was called after login
seem like the row_factory wasn't used

the reason was in the main session we used `execution_profiles`
while in the one create in login we did not, which led to copying
the wrong values (i.e. like `row_factory` that was needed for server
side describe)

this change saves the profiles into the instance, and reuse them
when ever a new session is opened
it simplify the code and we could remove a few repetitions of
the same logic
@Lorak-mmk Lorak-mmk self-assigned this Jun 1, 2024
@Lorak-mmk
Copy link
Collaborator

Before approving I want to properly understand this change.
Previously in do_login cqlsh was setting load_balancing_policy argument for Cluster constructor (using kwargs). IIUC now it's not needed because driver will take LBP from execution profile, that the version after your change now passes to Cluster in do_login. Is that correct?

Now what about those removed lines?

        session.default_timeout = self.session.default_timeout
        session.row_factory = self.session.row_factory
        session.default_consistency_level = self.session.default_consistency_level

I tried to follow default_timeout in driver code. It is set to 10.0 by default in the driver. I don't see any place in the driver where this value is changed, apart from the setter, and I don't see setter called anywhere in the driver or cqlsh. So I don't get how this value could change (and I assume it could if this code was necessary before your change). It has some interaction with execution profiles in Cluster.__init__:

        self.profile_manager.profiles[EXEC_PROFILE_DEFAULT] = ExecutionProfile(
            self.load_balancing_policy,
            self.default_retry_policy,
            request_timeout=Session._default_timeout,
            row_factory=Session._row_factory
        )

but I still can't understand the previous code - and I'd like to understand it before agreeing to remove it.

It's similar for default_consistency_level, but without setting it for execution profile in Cluster.__init__.

Regarding row_factory - there are a lot of occurrences in the driver so it's hard for me to follow. Please explain what it is and how it relates to execution profiles and why can this line be removed.

@fruch
Copy link
Collaborator Author

fruch commented Jun 1, 2024

@Lorak-mmk

The part you are missing is, that when profile is being used, you can set those values on session anymore, it would fail.

So that's why those have to be removed.

They are also the cause of the problem, cause we initial session was using profile, reading those values to set them into a new session is broken, cause they would be always default value, and not the values set in the profile.

@fruch
Copy link
Collaborator Author

fruch commented Jun 1, 2024

Also this bug isn't new, it was introduced in the initial version of our cqlsh fork

But now we first noticed it, when starting to use server side describe

@fruch fruch merged commit 0d58e5c into scylladb:master Jun 4, 2024
8 checks passed
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.

None yet

3 participants