duct-framework / database.sql.hikaricp

Integrant methods for a SQL database connection pool
5 stars 6 forks source link

Support resume and suspend #7

Closed kkharji closed 2 years ago

kkharji commented 2 years ago

Hey @weavejester this a quick PR that adds logging to state changes and it is very important in debugging.

Thanks

weavejester commented 2 years ago

Can you explain what you're trying to achieve? What is the change this PR implements, and why is it useful to you?

kkharji commented 2 years ago

Can you explain what you're trying to achieve? What is the change this PR implements, and why is it useful to you?

Sure.

Why?

Changes made in this PR:

  1. log changes on state, i.e. report when :duct.database.sql/hikaricp is initialized, halted, suspended and resumed.
  2. add logger* atom as work around to not being able to access logger in halt-key! and suspend-key! (hacky)
  3. modify tests to account from the report at initialization.
  4. only initialize :duct.database.sql/hikaricp when URI changes (not quite sure if other values should be accounted for)
kkharji commented 2 years ago

Not sure if in production logging database uri will be security risk though :D what do u think?

weavejester commented 2 years ago

It looks like this is two changes then: one to add logging, and one to add a resume mechanic. This should ideally be two PRs, as each PR should address only one change.

The logger should be attached to the boundary if we want to use it again, rather than using an atom.

The logs should not include the URI for exactly the reason you mention (security).

What bug did you encounter that would have been made more solvable if you had these logs? Why choose :report as the logging level instead of :debug?

How much time does the resume mechanic save? If it's short enough not to be noticeable by humans, then it's better to go with the more reliable option of halting and restarting.

kkharji commented 2 years ago

How much time does the resume mechanic save? If it's short enough not to be noticeable by humans, then it's better to go with the more reliable option of halting and restarting.

Yah, I haven't yet measured that. Not sure how I'd start? time function sufficient?. However, regardless of performance gain, isn't a good thing to try minimize the number of call external api methods that might not be "totally pure"?

weavejester commented 2 years ago

However, regardless of performance gain, isn't a good thing to try minimize the number of call external api methods that might not be "totally pure"?

No, because it adds uncertainty into the system. Ideally we want no difference between production and development environments, because then we can be more certain that if it works in development it works in production. In practice we can compromise for services that are problematic or costly to restart - the question is whether this service falls into that category.