Skip to content

HOTFIX: Fix null pointer when getting metric value in MetricsReporter#11248

Merged
ableegoldman merged 4 commits intoapache:trunkfrom
PhilHardwick:minor-fix-null-pointer-alive-stream-threads-metric
Aug 23, 2021
Merged

HOTFIX: Fix null pointer when getting metric value in MetricsReporter#11248
ableegoldman merged 4 commits intoapache:trunkfrom
PhilHardwick:minor-fix-null-pointer-alive-stream-threads-metric

Conversation

@PhilHardwick
Copy link
Copy Markdown
Contributor

@PhilHardwick PhilHardwick commented Aug 21, 2021

The alive stream threads metric relies on the threads field as a monitor object for
its synchronized block. When the alive stream threads metric is registered it isn't
initialised so any call to get the metric value before it is initialised will result
in a null pointer exception.

This is tested in a minimal integration test where the KafkaStreams object is created
but not started and the test loops through all metric values to check they're not null.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

The alive stream threads metric relies on the threads field as a monitor object for
its synchronized block. When the alive stream threads metric is registered it isn't
initialised so any call to get the metric value before it is initialised will result
in a null pointer exception.
@PhilHardwick
Copy link
Copy Markdown
Contributor Author

Requesting review from @ableegoldman and @wcarlson5 please

Copy link
Copy Markdown
Contributor

@wcarlson5 wcarlson5 left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable fix

Copy link
Copy Markdown
Member

@ableegoldman ableegoldman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix!

@ableegoldman ableegoldman changed the title MINOR: Fix null pointer when getting metric value in MetricsReporter HOTFIX: Fix null pointer when getting metric value in MetricsReporter Aug 23, 2021
@ableegoldman ableegoldman merged commit a594747 into apache:trunk Aug 23, 2021
ableegoldman pushed a commit that referenced this pull request Aug 23, 2021
…#11248)

The alive stream threads metric relies on the threads field as a monitor object for
its synchronized block. When the alive stream threads metric is registered it isn't
initialised so any call to get the metric value before it is initialised will result
in a null pointer exception.

Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>, Walker Carlson <wcarlson@confluent.io>
@ableegoldman
Copy link
Copy Markdown
Member

Merged to trunk and cherrypicked to 2.8

@kkonstantine
Copy link
Copy Markdown
Contributor

Approved for 3.0 as well.

ableegoldman pushed a commit that referenced this pull request Aug 23, 2021
…#11248)

The alive stream threads metric relies on the threads field as a monitor object for
its synchronized block. When the alive stream threads metric is registered it isn't
initialised so any call to get the metric value before it is initialised will result
in a null pointer exception.

Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>, Walker Carlson <wcarlson@confluent.io>
@ableegoldman
Copy link
Copy Markdown
Member

Cherrypicked to 3.0

@jolshan
Copy link
Copy Markdown
Member

jolshan commented Aug 24, 2021

Hi. This PR seemed to break the 2.8 branch. When I try to run checkstyle I see:

> Task :streams:compileTestJava
/Users/jolshan/kafka/streams/src/test/java/org/apache/kafka/streams/integration/MetricsReporterIntegrationTest.java:73: error: stop() has private access in EmbeddedKafkaCluster
        CLUSTER.stop();

I believe that file was added in this commit.

@ableegoldman
Copy link
Copy Markdown
Member

@jolshan ah, whoops. Thanks for the heads up. I'll get a patch ready

@ableegoldman
Copy link
Copy Markdown
Member

Fix is available here: #11257

This also got me digging into why we had to make stop() public in 3.0, and seem to now be required to manually invoke both start() and stop() in every single integration test...which is incredibly error prone (or rather, resource-leakage prone). I fixed the 2.8 build in a way that should prevent future backports from breaking the 2.8 branch specifically, but I'm going to look into just improving how we do this in 3.0+ so we don't need to worry about breaking older branches than 2.8, or leaking resources by forgetting to clean up this cluster... 😬

Anyways, thanks very much for bringing this up!

ableegoldman added a commit that referenced this pull request Aug 25, 2021
…uster (#11257)

A backport of #11248 broke the 2.8 build due to usage of the EmbeddedKafkaCluster#stop method, which used to be private. It seems we made this public when we upgraded to JUnit5 on the 3.0 branch and had to remove the ExternalResource that was previously responsible for calling start() and stop() for this class using the no-longer-available @ClassRule annotation.

Rather than adapt this test to the 2.8 style by migrating it to use @ClassRule as well, I opted to just make the stop() method public as well (since its analogue start()` has always been public anyways). This should hopefully prevent any future backports that include integration tests from having to manually go in and adapt the test, or accidentally break the build as happened here.

Reviewers: Luke Chen <showuon@gmail.com>
xdgrulez pushed a commit to xdgrulez/kafka that referenced this pull request Dec 22, 2021
…apache#11248)

The alive stream threads metric relies on the threads field as a monitor object for
its synchronized block. When the alive stream threads metric is registered it isn't
initialised so any call to get the metric value before it is initialised will result
in a null pointer exception.

Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>, Walker Carlson <wcarlson@confluent.io>
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.

5 participants