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
feat(spanner): Support fine grained access control #19067
Conversation
- let user create client with database role
@@ -277,8 +277,7 @@ def can_allocate_more_sessions? | |||
end | |||
|
|||
def create_keepalive_task! | |||
@keepalive_task = Concurrent::TimerTask.new(execution_interval: 300, | |||
timeout_interval: 60) do | |||
@keepalive_task = Concurrent::TimerTask.new execution_interval: 300 do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing timeout_interval
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timeout_interval is not taken into account by concurrent_ruby now https://github.com/ruby-concurrency/concurrent-ruby/blob/4cfc0a107b10d3ab213396d53411a1ee487200af/lib/concurrent-ruby/concurrent/timer_task.rb#L268 .. using it puts a lot of unnecessary warnings when running tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks. Can we just add a comment here for that?
Overall LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer adding it in the commit description when merging,
As we are anyways removing the option. Does that sound fine ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple nits, otherwise LGTM.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
bundle exec rake ci
in the gem subdirectory.closes: #<issue_number_goes_here>