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.

Sign up for GitHub

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

Jump to bottom

added a timestamp.timezone configuration in SplunkSinkConnectorConfig #403

Merged
merged 9 commits into from
Sep 5, 2023
Merged

added a timestamp.timezone configuration in SplunkSinkConnectorConfig #403

merged 9 commits into from
Sep 5, 2023

Conversation

mo-mosh
Copy link
Contributor

@mo-mosh mo-mosh commented Aug 28, 2023

I have the same issue "Add support for timezone override #398", so I added a timestamp.timezone.
as a default it's the UTC, but in my case I needed to change UTC to Asia/Seoul.

@mo-mosh mo-mosh temporarily deployed to workflow-approval August 28, 2023 02:58 — with GitHub Actions Inactive
VihasMakwana
VihasMakwana reviewed Aug 31, 2023
View reviewed changes
src/main/java/com/splunk/kafka/connect/SplunkSinkTask.java Outdated
@@ -578,9 +578,12 @@ private void timestampExtraction(Event event) {
log.warn("Could not set the time", e);
}
} else {
SimpleDateFormat df = new SimpleDateFormat(connectorConfig.timestampFormat);
SimpleDateFormat df = new SimpleDateFormat(connectorConfig.timestampFormat,Locale.ENGLISH);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need locale change?

Copy link
Contributor Author

@mo-mosh mo-mosh Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

without Locale information, in my local build, I get failed from test proceed, because orginally locale set up from system(os) as ko_KR. so I get exception error like this "Unparseable date: "Jun 13 2010 23:11:52.454 UTC".
(there's a English Jun)
I believe that the timestamp format of testcode should be changed or this code needs to be added Locale.ENGLISH.

but that's not my testcode, so I will remove Locale.ENGLISH as it was

VihasMakwana
VihasMakwana reviewed Aug 31, 2023
View reviewed changes
src/test/java/com/splunk/kafka/connect/SplunkSinkTaskTest.java Outdated
@@ -257,6 +257,7 @@ public void checkExtractedTimestamp() {
config.put(SplunkSinkConnectorConfig.ENABLE_TIMESTAMP_EXTRACTION_CONF, String.valueOf(true));
config.put(SplunkSinkConnectorConfig.REGEX_CONF, "\\\"time\\\":\\s*\\\"(?<time>.*?)\"");
config.put(SplunkSinkConnectorConfig.TIMESTAMP_FORMAT_CONF, "MMM dd yyyy HH:mm:ss.SSS zzz");
// config.put(SplunkSinkConnectorConfig.TIMESTAMP_TIMEZONE_CONF, "");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove it, as it defaults to empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I agree =)

@github-actions
Copy link

github-actions bot commented Aug 31, 2023 •
edited
Loading

Unit Test Results

167 tests   167 ✔️  38s ⏱️
  23 suites      0 💤
  23 files        0 ❌

Results for commit 1c6a738.

♻️ This comment has been updated with latest results.

@VihasMakwana
Copy link
Contributor

Can you add test cases covering your config?

@VihasMakwana
Copy link
Contributor

@mo-mosh sync develop with your branch, it involves a fix for failing pipeline.

@VihasMakwana
Copy link
Contributor

@mo-mosh I can add test cases, if you're occupied.
I'm pushing for this because we (splunk) are planning to release a new minor version with some fixes.
I would love to have this feature in our latest release.

@VihasMakwana
Copy link
Contributor

Hope you don't mind if I raise a PR against your repo?

@mo-mosh
Copy link
Contributor Author

mo-mosh commented Sep 3, 2023

I don't mind =) and I'll update repo and gonna add test cases
sorry for the late reply !

@mo-mosh mo-mosh temporarily deployed to workflow-approval September 4, 2023 07:31 — with GitHub Actions Inactive
@mo-mosh
Copy link
Contributor Author

mo-mosh commented Sep 4, 2023

I updated the code like I added only what I need from the origin develop branch
please review the code @VihasMakwana

@VihasMakwana VihasMakwana self-requested a review September 4, 2023 10:59
VihasMakwana
VihasMakwana reviewed Sep 4, 2023
View reviewed changes
Copy link
Contributor

@VihasMakwana VihasMakwana left a comment

Choose a reason for hiding this comment

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

@mo-mosh some comments.

src/test/java/com/splunk/kafka/connect/SplunkSinkTaskTest.java Outdated
config.put(SplunkSinkConnectorConfig.ENABLE_TIMESTAMP_EXTRACTION_CONF, String.valueOf(true));
config.put(SplunkSinkConnectorConfig.REGEX_CONF, "\\\"REQ_TIME\\\":\\s*\\\"(?<time>.*?)\"");
config.put(SplunkSinkConnectorConfig.TIMESTAMP_FORMAT_CONF, "yyyyMMddHHmmssSSS");
config.put(SplunkSinkConnectorConfig.TIMESTAMP_TIMEZONE_CONF, "Asis/Seoul");
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
config.put(SplunkSinkConnectorConfig.TIMESTAMP_TIMEZONE_CONF, "Asis/Seoul");
config.put(SplunkSinkConnectorConfig.TIMESTAMP_TIMEZONE_CONF, "Asia/Seoul");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my typo 😭

src/test/java/com/splunk/kafka/connect/SplunkSinkTaskTest.java Outdated Show resolved Hide resolved
@mo-mosh mo-mosh temporarily deployed to workflow-approval September 5, 2023 02:30 — with GitHub Actions Inactive
@VihasMakwana VihasMakwana self-requested a review September 5, 2023 06:23
VihasMakwana
VihasMakwana approved these changes Sep 5, 2023
View reviewed changes
Copy link
Contributor

@VihasMakwana VihasMakwana left a comment

Choose a reason for hiding this comment

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

LGTM

@VihasMakwana VihasMakwana merged commit 44c9d9a into splunk:develop Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VihasMakwana VihasMakwana VihasMakwana approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mo-mosh @VihasMakwana