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

Store table initialization #2583

Closed
sgnrslv opened this issue Oct 17, 2022 · 3 comments · Fixed by #2584
Closed

Store table initialization #2583

sgnrslv opened this issue Oct 17, 2022 · 3 comments · Fixed by #2584

Comments

@sgnrslv
Copy link
Contributor

sgnrslv commented Oct 17, 2022

This code redefines store table provided via --store_table or $MICRO_STORE_TABLE for no reason.
And forces us to re-init store table later once again.

I think the previous approach (before the commit ) was a correct one. What was the reason for this change?

@Davincible
Copy link
Contributor

Honestly no clue. The easiest way to resolve this is by adding an extra option with a check to skip this. Would you be willing to create a PR?

@sgnrslv
Copy link
Contributor Author

sgnrslv commented Oct 20, 2022

An extra option seems superfluous since providing store table via option or env variable can be treated already as an intention to have this exact table name.

I think the change was kind of an ad-hoc to remove the table specifying in cocroach plugin. But I see that this is not a problem anymore in the plugin.

So it seems this will be OK:

		// If the store has no Table set, fallback to the
		// services name
		if len(store.DefaultStore.Options().Table) == 0 {
			name := s.opts.Cmd.App().Name
			store.DefaultStore.Init(store.Table(name))
		}

i.e. as it was before.

@Davincible
Copy link
Contributor

You're right. Could you make a PR?

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 a pull request may close this issue.

2 participants