scylladb / scylla-cqlsh

A fork of the cqlsh code
Apache License 2.0
11 stars 29 forks source link

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

Closed fruch closed 1 month ago

fruch commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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