LiamClarkeNZ commented on code in PR #20402:
URL: https://github.com/apache/kafka/pull/20402#discussion_r2403728622
##########
connect/transforms/src/main/java/org/apache/kafka/connect/transforms/TimestampConverter.java:
##########
@@ -83,7 +86,8 @@ public abstract class TimestampConverter<R extends
ConnectRecord<R>> implements
private static final String UNIX_PRECISION_NANOS = "nanoseconds";
private static final String UNIX_PRECISION_SECONDS = "seconds";
- private static final TimeZone UTC = TimeZone.getTimeZone("UTC");
+ private static TimeZone UTC = TimeZone.getTimeZone("UTC");
+ //private static final TimeZone UTC =
TimeZone.getTimeZone("Asia/Shanghai");
Review Comment:
Dead code. Also, please re-add the `final` keyword to UTC.
##########
connect/transforms/src/main/java/org/apache/kafka/connect/transforms/TimestampConverter.java:
##########
@@ -68,6 +68,9 @@ public abstract class TimestampConverter<R extends
ConnectRecord<R>> implements
public static final String UNIX_PRECISION_CONFIG = "unix.precision";
private static final String UNIX_PRECISION_DEFAULT = "milliseconds";
+ public static final String TIMEZONE_CONFIG = "time_zone";
Review Comment:
```suggestion
public static final String TIMEZONE_CONFIG = "time.zone";
```
Consistent naming please :)
##########
connect/transforms/src/main/java/org/apache/kafka/connect/transforms/TimestampConverter.java:
##########
@@ -271,16 +277,18 @@ public Date toType(Config config, Date orig) {
// This is a bit unusual, but allows the transformation config to be
passed to static anonymous classes to customize
// their behavior
private static class Config {
- Config(String field, String type, SimpleDateFormat format, String
unixPrecision) {
+ Config(String field, String type, SimpleDateFormat format, String
unixPrecision, String time_zone) {
this.field = field;
this.type = type;
this.format = format;
this.unixPrecision = unixPrecision;
+ this.time_zone= time_zone;
Review Comment:
```suggestion
this.timeZone = timeZone;
```
##########
connect/transforms/src/main/java/org/apache/kafka/connect/transforms/TimestampConverter.java:
##########
@@ -271,16 +277,18 @@ public Date toType(Config config, Date orig) {
// This is a bit unusual, but allows the transformation config to be
passed to static anonymous classes to customize
// their behavior
private static class Config {
- Config(String field, String type, SimpleDateFormat format, String
unixPrecision) {
+ Config(String field, String type, SimpleDateFormat format, String
unixPrecision, String time_zone) {
Review Comment:
```suggestion
Config(String field, String type, SimpleDateFormat format, String
unixPrecision, String timeZone) {
```
Please keep casing consistent :)
##########
connect/transforms/src/main/java/org/apache/kafka/connect/transforms/TimestampConverter.java:
##########
@@ -293,12 +301,18 @@ public void configure(Map<String, ?> configs) {
final String type = simpleConfig.getString(TARGET_TYPE_CONFIG);
String formatPattern = simpleConfig.getString(FORMAT_CONFIG);
final String unixPrecision =
simpleConfig.getString(UNIX_PRECISION_CONFIG);
+ final String time_zone = simpleConfig.getString(TIMEZONE_CONFIG);
schemaUpdateCache = new SynchronizedCache<>(new LRUCache<>(16));
replaceNullWithDefault =
simpleConfig.getBoolean(REPLACE_NULL_WITH_DEFAULT_CONFIG);
if (type.equals(TYPE_STRING) && Utils.isBlank(formatPattern)) {
throw new ConfigException("TimestampConverter requires format
option to be specified when using string timestamps");
}
+
+ if (!Utils.isBlank(time_zone)) {
+ UTC = TimeZone.getTimeZone(time_zone);
Review Comment:
Please don't reassign a timezone that likely isn't UTC to a constant called
`UTC` :) I'd suggest using a new field.
##########
connect/transforms/src/main/java/org/apache/kafka/connect/transforms/TimestampConverter.java:
##########
@@ -92,6 +96,8 @@ public abstract class TimestampConverter<R extends
ConnectRecord<R>> implements
public static final ConfigDef CONFIG_DEF = new ConfigDef()
.define(FIELD_CONFIG, ConfigDef.Type.STRING, FIELD_DEFAULT,
ConfigDef.Importance.HIGH,
"The field containing the timestamp, or empty if the
entire value is a timestamp")
+ .define(TIMEZONE_CONFIG, ConfigDef.Type.STRING, TIMEZONE_DEFAULT,
ConfigDef.Importance.HIGH,
+ "The field containing the time_zone.")
Review Comment:
```suggestion
"The field containing the time zone.")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
