Skip to content

Fix issue "redis" engine is not registered#19239

Merged
greedy52 merged 3 commits intomasterfrom
STeve/register_redis_engine
Dec 9, 2022
Merged

Fix issue "redis" engine is not registered#19239
greedy52 merged 3 commits intomasterfrom
STeve/register_redis_engine

Conversation

@greedy52
Copy link
Copy Markdown
Contributor

@greedy52 greedy52 commented Dec 9, 2022

Error when connecting Redis database

Original Error: *trace.NotFoundError database engine "redis" is not registered

Related PR

(interestingly, this is not caught by test, likely because test has different importing path)

@greedy52 greedy52 requested a review from smallinsky December 9, 2022 14:41
@greedy52 greedy52 self-assigned this Dec 9, 2022
@greedy52 greedy52 added database-access Database access related issues and PRs db/redis backport/branch/v11 labels Dec 9, 2022
@github-actions github-actions Bot requested review from lxea and zmb3 December 9, 2022 14:41
Copy link
Copy Markdown
Contributor

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

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

Does the init call was for redis engine in previous version was called from https://github.com/gravitational/teleport/pull/18776/files#diff-054e31280a78f4aea4759cd740f8d8bcecf563679d7fe6b79991fdb944e8c611L64 ?

LGTM to me though I think that we should clean when and how our db engines are registered to prevent this to happen again.

@greedy52
Copy link
Copy Markdown
Contributor Author

greedy52 commented Dec 9, 2022

Does the init call was for redis engine in previous version was called from

Yes. We were calling redis.ParseRedisAddress before so the redis lib gets implicitly inited. With that change, the new connection.ParseRedisAddress is used instead so we have to explicitly import the lib now.

LGTM to me though I think that we should clean when and how our db engines are registered to prevent this to happen again.

Agreed. The test didn't catch the problem as the test has to import redis for other setups. Need to ensure this won't happen again. How about checking if every engine type is registered at server start(or New)?

@github-actions github-actions Bot removed the request for review from lxea December 9, 2022 16:10
@smallinsky
Copy link
Copy Markdown
Contributor

Agreed. The test didn't catch the problem as the test has to import redis for other setups. Need to ensure this won't happen again. How about checking if every engine type is registered at server start(or New)?

Yes that will work but I wonder if we can be more explict and insteam of calling _ db/engine/import we can just init everything from one place like:

// db/server.go
func init() {
  common.RegisterEngine(redis.NewEngine, defaults.ProtocolRedis)
  common.RegisterEngine(msql.NewEngine, defaults.ProtocolMySQL)
}

and makes things more readable.

@greedy52
Copy link
Copy Markdown
Contributor Author

greedy52 commented Dec 9, 2022

Yes that will work but I wonder if we can be more explict and insteam of calling _ db/engine/import we can just init everything from one place like:

// db/server.go
func init() {
  common.RegisterEngine(redis.NewEngine, defaults.ProtocolRedis)
  common.RegisterEngine(msql.NewEngine, defaults.ProtocolMySQL)
}

and makes things more readable.

@smallinsky really like that. Will do that separately. The test registers fake engine in init() so need to make sure the ordering.

@greedy52 greedy52 enabled auto-merge (squash) December 9, 2022 17:14
@greedy52 greedy52 merged commit 66a915f into master Dec 9, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Dec 9, 2022

@greedy52 See the table below for backport results.

Branch Result
branch/v11 Failed

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

Labels

database-access Database access related issues and PRs db/redis size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants