Skip to content

Enable to configure metrics-publisher in AWS client factory#15304

Open
rmannibucau wants to merge 3 commits intoapache:mainfrom
rmannibucau:dev/enable-to-configure-metrics-publisher-in-aws-client-factory
Open

Enable to configure metrics-publisher in AWS client factory#15304
rmannibucau wants to merge 3 commits intoapache:mainfrom
rmannibucau:dev/enable-to-configure-metrics-publisher-in-aws-client-factory

Conversation

@rmannibucau
Copy link

No description provided.

Copy link
Contributor

@steveloughran steveloughran left a comment

Choose a reason for hiding this comment

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

The PR #15122 also proposes a metrics publisher; it's probably best to see how that PR suits you and what bits of this you think need to be pulled in.

I like the tests here

factory.initialize(
Map.of(
// CRT impl is still not equivalent to the default java one
"s3.crt.enabled", "false",
Copy link
Contributor

Choose a reason for hiding this comment

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

just a source of a different stack traces...

Copy link
Author

Choose a reason for hiding this comment

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

not sure I got this one, issue is CRT doesn't support metrics publisher so I disable it explicitly in tests

@rmannibucau
Copy link
Author

#15122 works for me but doesnt expose any configuration - not a blocker here since I end up storing in a static copyonwritelist the instances to wire them in a spark sink but I assume we want to align the model on credential provider one?

No issue to close this PR if the other one is preferred while it hits a release at some point :)

@steveloughran
Copy link
Contributor

just suggesting you work together to produce something which you both find works, as that's how best to get something in which suits more people

@rmannibucau
Copy link
Author

@rcjverhoef do you want to add the properties filter + create() - noarg - support in your PR to align on credential provider pattern? Or do we align credential provider one on your PR which is passing all properties? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants