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

dlog: Fix issue where the fast-logging broke backward compat #33

Merged
merged 3 commits into from
May 24, 2022

Conversation

LukeShu
Copy link
Contributor

@LukeShu LukeShu commented May 23, 2022

This PR is on top of #34, go merge that first.

#28 changed the Logger interface, breaking any existing implementors of the interface. That's not OK.

So revert that interface (and add a comment about how it's frozen), and add an opt-in OptimizedLogger interface for Jose's optimizations.

@LukeShu LukeShu requested a review from josecv May 23, 2022 20:29
@LukeShu LukeShu changed the base branch from master to lukeshu/news-1.2 May 23, 2022 21:37
Base automatically changed from lukeshu/news-1.2 to release/v1.2 May 23, 2022 22:21
Copy link
Contributor

@thallgren thallgren left a comment

Choose a reason for hiding this comment

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

Since I consider the non-optimized logger a design flaw, I'd prefer if this PR wasn't merged and the version instead bumped to 2.0.0. That way the backward compatibility wouldn't be broken for 1.x users and the flaw could still be removed.

@@ -47,13 +50,38 @@ type Logger interface {
StdLogger(LogLevel) *log.Logger

// Log actually logs a message.
Log(level LogLevel, args ...interface{})
Log(level LogLevel, msg string)
Copy link
Contributor

Choose a reason for hiding this comment

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

Reverting this will break Telepresence, which I assume is OK, given that we're not using a released version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh dang, you guys finally upgraded?

Yeah, it's a bad situation because 1.2.4 has been around so long. Do you break the folks who have upgraded to 1.2.4, or break the folks who are on older versions?

Copy link
Contributor

@josecv josecv left a comment

Choose a reason for hiding this comment

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

this lgtm from a code perspective, i do kind of agree we might wanna release a 2.0.0 but im okay with unbreaking the 1.x line for now

@LukeShu
Copy link
Contributor Author

LukeShu commented May 24, 2022

Since I consider the non-optimized logger a design flaw

I don't think I agree. I think keeping the required-interface minimal and letting dlog handle as much complexity as it can is a good thing.

@LukeShu
Copy link
Contributor Author

LukeShu commented May 24, 2022

I've created a "v2" milestone and an issue within it to revisit the dlog.Logger interface: #35 (and also a few other issues that we'll want to consider if/when we do a v2)

@LukeShu LukeShu merged commit 5764846 into release/v1.2 May 24, 2022
@LukeShu LukeShu deleted the lukeshu/fix-dlog-regression branch May 24, 2022 19:13
@thallgren
Copy link
Contributor

A 2.0 version would be good for three reasons.

  1. It wouldn't require this somewhat cludgy solution with casts everywhere.
  2. It would give the 1.2 implementers a chance to simply stay put while at the same time give them an incentive to improve their implementation.
  3. It would give Telepresence a version that doesn't break us.

My motivation for calling the old version flawed is that unnecessary string manipulation is expensive and the added complexity is very trivial. In fact, in most cases it's just delegated to another log implementation.

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 this pull request may close these issues.

3 participants