Skip to content

Conversation

@yu-kioo
Copy link
Contributor

@yu-kioo yu-kioo commented Aug 7, 2024

overview

add optional user-agent parameter to JDBC and SDK connection

@yu-kioo yu-kioo changed the title add user-agent parameter Add user-agent parameter to config Aug 7, 2024
String name = task.getUserAgent().get().get("product_name");
String version = task.getUserAgent().get().get("product_version");

UserAgent.withProduct(name, version);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

return new DatabricksAPIClient(createDatabricksConfig(task));
}

private static void SetUserAgent(DatabricksOutputPlugin.DatabricksPluginTask task) {
Copy link
Contributor

Choose a reason for hiding this comment

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

普通にメソッドなので先頭は小文字でいい気がします!僕の知らないお作法だったらごめんなさい!

Suggested change
private static void SetUserAgent(DatabricksOutputPlugin.DatabricksPluginTask task) {
private static void setUserAgent(DatabricksOutputPlugin.DatabricksPluginTask task) {

@yu-kioo yu-kioo requested a review from NamedPython August 8, 2024 03:13
Copy link
Contributor

@d-hrs d-hrs left a comment

Choose a reason for hiding this comment

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

please check my comment.


@Config("user_agent")
@ConfigDefault("{}")
public Optional<UserAgentEntry> getUserAgent();
Copy link
Contributor

@d-hrs d-hrs Aug 8, 2024

Choose a reason for hiding this comment

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

Suggested change
public Optional<UserAgentEntry> getUserAgent();
public UserAgentEntry getUserAgentEntry();
  • [MAY] If there is a default value, then Optional is not needed.
  • [SHOULD] If there is no special reason, it should be the same as the option name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in this commit.

@yu-kioo yu-kioo requested a review from d-hrs August 8, 2024 09:02
props.put("ConnSchema", t.getSchemaName());
props.putAll(t.getOptions());
// overwrite UserAgentEntry property if the same property is set in options
if (t.getUserAgentEntry() != null) {
Copy link
Contributor

@chikamura chikamura Aug 10, 2024

Choose a reason for hiding this comment

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

t.getUserAgentEntry always seems to be not null.
You might want to delete this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current code did not guarantee the behavior of 2 below, so changed condition.

  • 1: options is not set & user-agent is not set => unknown/0.0.0
  • 2: options is set & user-agent is not set => value of options
  • 3: options is not set & user-agent is set => value of user-agent
  • 4: options is set, user-agent is set => value of user-agent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have decided to always overwrite the UserAgentEntry value in options by user-agent, so please ignore the above comment. I removed condition as you commented.

Comment on lines 112 to 115
String product_name = t.getUserAgentEntry().getProductName();
String product_version = t.getUserAgentEntry().getProductVersion();

props.put("UserAgentEntry", product_name + "/" + product_version);
Copy link
Contributor

Choose a reason for hiding this comment

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

Java's variable names are usually camel case.

product_name -> productName
product_version -> productVersion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, fixed.

@yu-kioo yu-kioo requested a review from chikamura August 14, 2024 04:41
@yu-kioo yu-kioo self-assigned this Aug 15, 2024
Copy link
Contributor

@chikamura chikamura left a comment

Choose a reason for hiding this comment

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

LGTM

@yu-kioo yu-kioo merged commit 213fa8a into main Aug 16, 2024
@yu-kioo yu-kioo deleted the add-user-agent-parameter-to-config branch August 16, 2024 00:48
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.

4 participants