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

Expose AWS HTTP Client stats via JMX #6503

Merged
merged 1 commit into from Jan 5, 2021

Conversation

losipiuk
Copy link
Member

@losipiuk losipiuk commented Jan 4, 2021

No description provided.

Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

Can this be tested, perhaps in TestHiveFileSystemS3?

@losipiuk
Copy link
Member Author

losipiuk commented Jan 5, 2021

Can this be tested, perhaps in TestHiveFileSystemS3?

I tried and I failed. It could be testable easily if we had PT referencing S3. Then we could verify the metrics using query over jmx.current.XXXXXX.

@findepi
Copy link
Member

findepi commented Jan 5, 2021

In TestHiveFileSystemS3 we don't have the server, and so we don't have the jmx connector, but maybe we can access the mbean server directly?

@losipiuk
Copy link
Member Author

losipiuk commented Jan 5, 2021

In TestHiveFileSystemS3 we don't have the server, and so we don't have the jmx connector, but maybe we can access the mbean server directly?

I spent more time on that. I can run the test. I see that stats callback are being called. But I cannot read JMX values as mbeans are not registered at all in tests. All the JMX mbean registration is happening via Guice automation which is not done by the test at all.
Even if I reworked the test to use Guice (which can be problematic) it is hard to write the test itself. I have have no control over pool state. I do not know how to predict expected pool size before and after test. With high probability we should expect same state as the HTTP connection would be created by other tests accessing S3, which are ran as part of this test class. Testing number of active HTTP connections is even more problematic as I would need to catch the HTTP request which is in-progress with an assertion.

I would say it is not worth it....

@findepi
Copy link
Member

findepi commented Jan 5, 2021

Thanks for investigating this.

@losipiuk losipiuk merged commit 39a9945 into trinodb:master Jan 5, 2021
@kokosing
Copy link
Member

kokosing commented Jan 5, 2021

. But I cannot read JMX values as mbeans are not registered at all in tests. All the JMX mbean registration is happening via Guice automation which is not done by the test at all.

That is typical issue when doing unit tests for JMX. Rule of thumb: it does not work. I have seen few examples of case where it worked but then such test has to be executed in separation as any other test can override mbeans or take precedence (not sure what actually happens).

The only advised way for JMX tests are product tests.

@losipiuk losipiuk mentioned this pull request Jan 28, 2021
10 tasks
@martint martint added this to the 352 milestone Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants