Skip to content

Conversation

sergiyvamz
Copy link
Contributor

@sergiyvamz sergiyvamz commented Jan 8, 2025

Summary

improve forceConnect pipeline:

  • review implemented forceConnect() for existing plugins
  • keep forceConnect() for plugins related to db authentication; remove forceConnect() for other plugins
  • allows to skip a particular plugin while calling connect() or forceConnect() pipeline

Additional Reviewers

@karenc-bq
@aaronchung-bitquill
@aaron-congo

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sergiyvamz sergiyvamz added the wip Pull request that is a work in progress label Jan 8, 2025
Copy link

github-actions bot commented Jan 8, 2025

Qodana Community for JVM

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

  1. Register at Qodana Cloud and configure the action
  2. Use GitHub Code Scanning with Qodana
  3. Host Qodana report at GitHub Pages
  4. Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/[email protected]
        with:
          upload-result: true
Contact Qodana team

Contact us at [email protected]

@sergiyvamz sergiyvamz removed the wip Pull request that is a work in progress label Jan 10, 2025
@sergiyvamz sergiyvamz force-pushed the force-connect-pipeline branch from a837402 to 18ecde9 Compare January 10, 2025 21:43
import software.amazon.jdbc.HostSpec;
import software.amazon.jdbc.JdbcCallable;
import software.amazon.jdbc.util.Messages;

public class ConnectTimeConnectionPlugin extends AbstractConnectionPlugin {
public class ConnectTimeConnectionPlugin extends AbstractConnectionPlugin
implements AuthenticationConnectionPlugin {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to my previous comment, it seems a bit misleading to label this as an Authentication plugin when it doesn't actually provide auth functionality. Can we similarly either remove this and explicitly check for this plugin in ConnectionPluginManager#makePluginChainFunc or do something like rename AuthenticationConnectionPlugin to ForceConnectPlugin?

import software.amazon.jdbc.HostSpec;
import software.amazon.jdbc.JdbcCallable;
import software.amazon.jdbc.plugin.AbstractConnectionPlugin;
import software.amazon.jdbc.util.StringUtils;
import software.amazon.jdbc.util.WrapperUtils;

public class DeveloperConnectionPlugin extends AbstractConnectionPlugin implements ExceptionSimulator {
public class DeveloperConnectionPlugin extends AbstractConnectionPlugin
implements AuthenticationConnectionPlugin, ExceptionSimulator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to my previous comment, it seems a bit misleading to label this as an Authentication plugin when it doesn't actually provide auth functionality. Can we similarly either remove this and explicitly check for this plugin in ConnectionPluginManager#makePluginChainFunc or do something like rename AuthenticationConnectionPlugin to ForceConnectPlugin

@aaron-congo
Copy link
Contributor

FastestResponseStrategyPlugin and BenchmarkPlugin still have a forceConnect method/implementation but will be excluded from the forceConnect pipeline with these changes. If they should be in the forceConnect pipeline, can we check for them in ConnectionPluginManager#makePluginChainFunc similarly to DefaultConnectionPlugin? And if they should not be in the forceConnect pipeline, can we remove their forceConnect/connectInternal methods?

@sergiyvamz sergiyvamz force-pushed the force-connect-pipeline branch from 254473d to ac548ae Compare January 14, 2025 17:45
@sergiyvamz sergiyvamz force-pushed the force-connect-pipeline branch from ac548ae to b4da891 Compare January 14, 2025 22:00
@sergiyvamz sergiyvamz merged commit b66e33d into main Jan 14, 2025
6 checks passed
@sergiyvamz sergiyvamz deleted the force-connect-pipeline branch January 14, 2025 23:01
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.

3 participants