Skip to content

Noop scope - #40

Merged
justinjc merged 6 commits into
uber-java:masterfrom
JigarJoshi:noop_scope
Sep 18, 2019
Merged

Noop scope#40
justinjc merged 6 commits into
uber-java:masterfrom
JigarJoshi:noop_scope

Conversation

@JigarJoshi

@JigarJoshiJigarJoshi commented May 15, 2019

Copy link
Copy Markdown
Contributor

@JigarJoshi

JigarJoshi commented May 15, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

This PR will fail around Java7 code compatibility. Do we really need that? @justinjc@prateek

@willsalz

willsalz commented Sep 4, 2019

Copy link
Copy Markdown

@JigarJoshi do you know who should be code reviewing this?

willsalz
willsalz reviewed Sep 4, 2019
View reviewed changes
Comment threadcore/src/main/java/com/uber/m3/tally/NoopScope.java Outdated

@Override
public Counter counter(String name) {
return (delta) -> {

@willsalzwillsalzSep 4, 2019•
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this might create a new lambda object every time [ not sure it the compiler will inline this ]. you could instantiate a static function protected final Provider<Counter> PROVIDER_FUNCTION = () -> {} and reuse

@willsalzwillsalzSep 4, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same goes for all of the other NoOp methods

@JigarJoshiJigarJoshiSep 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

good point. I updated the PR.

@JigarJoshi

JigarJoshi commented Sep 4, 2019

Copy link
Copy Markdown
ContributorAuthor

@prateek and @justinjc - We need to drop Java 7 support here and upgrade this build to be minimum at Java 8.

willsalz
willsalz approved these changes Sep 4, 2019
View reviewed changes

@willsalzwillsalz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ship it! I didn't see any other reviewers active on this diff, and I'm not sure who actively watches this project, but feel free to land and we can improve as needed.

Comment threadcore/src/main/java/com/uber/m3/tally/NoopScope.java Outdated

@Override
public Counter counter(String name) {
return (delta) -> {

@willsalzwillsalzSep 4, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same goes for all of the other NoOp methods

Comment threadm3/src/main/java/com/uber/m3/tally/m3/M3Reporter.java
Comment threadcore/src/main/java/com/uber/m3/tally/NoopScope.java
@JigarJoshiJigarJoshi mentioned this pull request Sep 9, 2019
Open
@justinjc

justinjc commented Sep 17, 2019

Copy link
Copy Markdown
Collaborator

@JigarJoshi looks mostly good, thanks! Looks like the build is red because checkstyle is complaining about the missing copyright statement at the beginning of the file. Can you put that in please? Copy it from some other file but use 2019 as the year.

FYI when it says something like :tally-core:checkstyleMain FAILED in the build, you can run ./gradlew :tally-core:checkstyleMain to check locally that you've fixed the issue before re-pushing.

@justinjc
justinjc merged commit 6394c58 into uber-java:masterSep 18, 2019
@justinjcjustinjc mentioned this pull request Sep 18, 2019
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@willsalzwillsalzwillsalz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JigarJoshi@willsalz@justinjc