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

Auth: Enable retries and transaction for some db calls for auth tokens #16785

Merged
merged 6 commits into from
Apr 30, 2019

Conversation

bergquist
Copy link
Contributor

ref #16638

the WithSession wrapper handles retries and connection
management so the caller dont have to worry about it.
@bergquist bergquist requested a review from marefr April 26, 2019 13:26
@bergquist
Copy link
Contributor Author

Migrating from the old way to using WithDbSession I realized how bad the API is. We need to figure out a better API for this. Its very easy to write code like this

var affected int64
err := srv.SQLStore.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error {
	// code

	affected, err := res.RowsAffected() 
	return err
})

and create an new variable in that scope instead of assigning the original one.

Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Just to be clear this doesn't add any retries for sqlite right?

You did a very thorough work getting context in everywhere, but in auth_token.go you forget to use the provided ctx as argument to WithDbSession :)

pkg/services/auth/auth_token.go Outdated Show resolved Hide resolved
pkg/services/auth/auth_token.go Outdated Show resolved Hide resolved
pkg/services/auth/auth_token.go Outdated Show resolved Hide resolved
pkg/services/auth/auth_token.go Outdated Show resolved Hide resolved
pkg/services/auth/auth_token.go Outdated Show resolved Hide resolved
pkg/services/auth/auth_token.go Outdated Show resolved Hide resolved
pkg/services/auth/auth_token.go Outdated Show resolved Hide resolved
pkg/services/auth/auth_token.go Outdated Show resolved Hide resolved
pkg/services/auth/auth_token.go Outdated Show resolved Hide resolved
pkg/services/auth/auth_token.go Outdated Show resolved Hide resolved
@torkelo
Copy link
Member

torkelo commented Apr 26, 2019

Just to understand the db locked issue and how this solves it. Was it caused, you suspect, by opening & not closing sessions?

Wonder if it would be possible to open a session in the middleware at the start and store it in the Request context, and the pass context down to sql layer and have the withDbSession reuse session in the context object. And have the middleware close the session on request completion.

@bergquist
Copy link
Contributor Author

Fixed the context.Background issues and switched to using transactions (inc retries) for rotation. I was considering adding retries for all WithDbSession requests as well but Im not sure about it. It could potentially create more requests to the db if it's exhausted.

@torkelo yesterday I didn't like that idea but I might have changed my mind. We still need some way of managing session background tasks. Let's discuss that in another issue. I think we need to improve this soon as more people will be working in the backend.

Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Everything looks good besides some dummy code that seems to have been sneaked in, see comment.

fmt.Println("kasdgfkjhgsdafkjhgasdkjhf")
fmt.Println("kasdgfkjhgsdafkjhgasdkjhf")
fmt.Println("kasdgfkjhgsdafkjhgasdkjhf")

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha

@bergquist bergquist changed the title Use sqlstore.WithDbSession in auth token package Auth: Enable retries and transaction for some db calls for auth tokens Apr 30, 2019
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

🎉

@bergquist bergquist merged commit 9660356 into grafana:master Apr 30, 2019
@bergquist bergquist deleted the auth_token_retries branch April 30, 2019 12:42
ryantxu added a commit to ryantxu/grafana that referenced this pull request Apr 30, 2019
…naui

* grafana/master:
  Auth: Enable retries and transaction for some db calls for auth tokens  (grafana#16785)
  Provisioning: Show file path of provisioning file in save/delete dialogs (grafana#16706)
  Add tracing headers for prometheus datasource (grafana#16724)
  Config: Fixes bug where timeouts for alerting was not parsed correctly (grafana#16784)
  build: removes gopkg files from dev docker file (grafana#16817)
  Provisioning: Trying to fix failing test (grafana#16800)
  Table: React table fix rotate support in storybook (grafana#16816)
  TestData: add log level in dummy message (grafana#16815)
  removes gopkg.lock from root folder
ryantxu added a commit to ryantxu/grafana that referenced this pull request Apr 30, 2019
* grafana/master:
  Auth: Enable retries and transaction for some db calls for auth tokens  (grafana#16785)
  Provisioning: Show file path of provisioning file in save/delete dialogs (grafana#16706)
  Add tracing headers for prometheus datasource (grafana#16724)
  Config: Fixes bug where timeouts for alerting was not parsed correctly (grafana#16784)
  build: removes gopkg files from dev docker file (grafana#16817)
  Provisioning: Trying to fix failing test (grafana#16800)
  Table: React table fix rotate support in storybook (grafana#16816)
  TestData: add log level in dummy message (grafana#16815)
  removes gopkg.lock from root folder
  Explore: Support user timezone (grafana#16469)
  Plugins: rename vizPlugin to panelPlugin (grafana#16802)
  Plugins: move app/feature/plugin properties into PluginMeta (grafana#16809)
  Plugins: move PanelPluginMeta to grafana/ui (grafana#16804)
  Plugins: move datasource specific meta out of the main meta type (grafana#16803)
  updates changelog for 6.1.6
  Fix: Fetch histogram series from other api route (grafana#16768)
  phantomjs: set web-security to true
  Chore: Lowered implicit anys limit to 5668
ryantxu added a commit to ryantxu/grafana that referenced this pull request Apr 30, 2019
…MetricPanelCtrl

* grafana/master:
  Explore: Use SeriesData format for loki/logs (grafana#16793)
  Refactor: move NavModel to @grafana/ui (grafana#16813)
  Auth: Enable retries and transaction for some db calls for auth tokens  (grafana#16785)
  Provisioning: Show file path of provisioning file in save/delete dialogs (grafana#16706)
  Add tracing headers for prometheus datasource (grafana#16724)
  Config: Fixes bug where timeouts for alerting was not parsed correctly (grafana#16784)
  build: removes gopkg files from dev docker file (grafana#16817)
  Provisioning: Trying to fix failing test (grafana#16800)
  Table: React table fix rotate support in storybook (grafana#16816)
  TestData: add log level in dummy message (grafana#16815)
  removes gopkg.lock from root folder
  Explore: Support user timezone (grafana#16469)
  Plugins: rename vizPlugin to panelPlugin (grafana#16802)
  Plugins: move app/feature/plugin properties into PluginMeta (grafana#16809)
  Plugins: move PanelPluginMeta to grafana/ui (grafana#16804)
  Plugins: move datasource specific meta out of the main meta type (grafana#16803)
  updates changelog for 6.1.6
  Fix: Fetch histogram series from other api route (grafana#16768)
  phantomjs: set web-security to true
  Chore: Lowered implicit anys limit to 5668
ryantxu added a commit to ryantxu/grafana that referenced this pull request Apr 30, 2019
* grafana/master:
  Explore: Use SeriesData format for loki/logs (grafana#16793)
  Refactor: move NavModel to @grafana/ui (grafana#16813)
  Auth: Enable retries and transaction for some db calls for auth tokens  (grafana#16785)
  Provisioning: Show file path of provisioning file in save/delete dialogs (grafana#16706)
  Add tracing headers for prometheus datasource (grafana#16724)
  Config: Fixes bug where timeouts for alerting was not parsed correctly (grafana#16784)
  build: removes gopkg files from dev docker file (grafana#16817)
  Provisioning: Trying to fix failing test (grafana#16800)
  Table: React table fix rotate support in storybook (grafana#16816)
  TestData: add log level in dummy message (grafana#16815)
  removes gopkg.lock from root folder
  Explore: Support user timezone (grafana#16469)
  Plugins: rename vizPlugin to panelPlugin (grafana#16802)
  Plugins: move app/feature/plugin properties into PluginMeta (grafana#16809)
  Plugins: move PanelPluginMeta to grafana/ui (grafana#16804)
  Plugins: move datasource specific meta out of the main meta type (grafana#16803)
  updates changelog for 6.1.6
  Fix: Fetch histogram series from other api route (grafana#16768)
  phantomjs: set web-security to true
  Chore: Lowered implicit anys limit to 5668
ryantxu added a commit to ryantxu/grafana that referenced this pull request May 2, 2019
* grafana/master: (27 commits)
  FormLabel: allow any rather than just a string for tooltip (grafana#16841)
  prometheus: fix regression after adding support for tracing headers (grafana#16829)
  area/circleci: Speed up circleci build process for branches and pr (grafana#16778)
  DataProxy: Restore Set-Cookie header after proxy request (grafana#16838)
  docs: clarify page parameter version support for folder/dashboard search (grafana#16836)
  Chore: revise some of the gosec rules (grafana#16713)
  Refactor: consistant plugin/meta usage (grafana#16834)
  Explore: Use SeriesData format for loki/logs (grafana#16793)
  Refactor: move NavModel to @grafana/ui (grafana#16813)
  Auth: Enable retries and transaction for some db calls for auth tokens  (grafana#16785)
  Provisioning: Show file path of provisioning file in save/delete dialogs (grafana#16706)
  Add tracing headers for prometheus datasource (grafana#16724)
  Config: Fixes bug where timeouts for alerting was not parsed correctly (grafana#16784)
  build: removes gopkg files from dev docker file (grafana#16817)
  Provisioning: Trying to fix failing test (grafana#16800)
  Table: React table fix rotate support in storybook (grafana#16816)
  TestData: add log level in dummy message (grafana#16815)
  removes gopkg.lock from root folder
  Explore: Support user timezone (grafana#16469)
  Plugins: rename vizPlugin to panelPlugin (grafana#16802)
  ...
@bergquist bergquist added this to the 6.2-beta1 milestone May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants