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

Remove legacy ClientConfiguration code #3074

Merged
merged 4 commits into from Nov 10, 2022

Conversation

ctubbsii
Copy link
Member

@ctubbsii ctubbsii commented Nov 9, 2022

  • Delete ClientConfiguration and unit test
  • Update ClientConfConverter to remove legacy client config
  • Update ClientConfConverterTest.basic to test conversion directly to/from client Properties and AccumuloConfiguration (which is still needed for some bootstrapping code and SASL configuration code)
  • Remove legacy trace props from ClientProperty
  • Remove deprecated getClientConfig() methods from minicluster
  • No longer write out client config file (only client properties file) and no longer set corresponding ACCUMULO_CLIENT_CONF_PATH environment variable for minicluster
  • Update SaslServerConnectionParams to remove unused overloaded constructors and setters (allowing secretManager field to be final); this wasn't strictly necessary in this patch, but helped simplify things to make it easier to trace the uses of the ClientConfConverter methods to see how they were still needed

* Delete ClientConfiguration and unit test
* Update ClientConfConverter to remove legacy client config
* Update ClientConfConverterTest.basic to test conversion directly
  to/from client Properties and AccumuloConfiguration (which is still
  needed for some bootstrapping code and SASL configuration code)
* Remove legacy trace props from ClientProperty
* Remove deprecated getClientConfig() methods from minicluster
* No longer write out client config file (only client properties file)
  and no longer set corresponding `ACCUMULO_CLIENT_CONF_PATH`
  environment variable for minicluster
* Update SaslServerConnectionParams to remove unused overloaded
  constructors and setters (allowing secretManager field to be final);
  this wasn't strictly necessary in this patch, but helped simplify
  things to make it easier to trace the uses of the ClientConfConverter
  methods to see how they were still needed
@ctubbsii ctubbsii self-assigned this Nov 9, 2022
@ctubbsii ctubbsii added this to In progress in 3.0.0 via automation Nov 9, 2022
Copy link
Contributor

@EdColeman EdColeman left a comment

Choose a reason for hiding this comment

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

LGTM - looks like there is additional work needed for the tests to pass though

Also ensure SASL Kerberos primary is handled correctly, update some unit
tests, and drop some more legacy credential provider client
configuration code, and simplify the AccumuloConfiguration anonymous
inner class by computing the conversion of the kerberos primary right
away, and only once.
@ctubbsii
Copy link
Member Author

This was very frustrating code to clean up, but I'm confident I've fixed the earlier bugs. I've already confirmed all the hung and failing tests from the first pass are fixed, but I will re-run the full ITs to be sure there's nothing else.

Fix some tests that apparently use the ClientConfConverter class to test
loading properties via a credential manager.
I can't reproduce the failure in GitHub locally
@ctubbsii
Copy link
Member Author

Full ITs finished successfully. Seems unrelated, but saw SaslConnectionParamsTest was a bit flaky in GitHub Actions. I forced it to run in a separate JVM fork from other tests, because it always worked for me when running on its own, so it seems likely another test in the same JVM was interfering with it.

@ctubbsii ctubbsii merged commit a7c0575 into apache:main Nov 10, 2022
3.0.0 automation moved this from In progress to Done Nov 10, 2022
@ctubbsii ctubbsii deleted the remove-legacy-client-config branch November 10, 2022 15:38
ctubbsii added a commit to ctubbsii/accumulo that referenced this pull request Feb 9, 2023
Resolve conflicts of apache#3180 and apache#3074 in MiniAccumuloClusterImpl to merge
PR apache#3180 into the main branch
asfgit pushed a commit that referenced this pull request Aug 14, 2023
* Bump ZK to 3.8.2 patch release
* Backport test/build improvements from #3074 and #3080
* Include updates to post vote checklist GitHub issue template from main
  branch to reduce differences (has no effect on branches other than
  main)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
3.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants