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

HIVE-26363: converting replLogger time format from epoch to readable UTC format #3439

Closed
wants to merge 0 commits into from

Conversation

Rakshith606
Copy link
Contributor

@Rakshith606 Rakshith606 commented Jul 14, 2022

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Copy link
Contributor

@cmunkey cmunkey left a comment

Choose a reason for hiding this comment

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

Are this timestamps going into the metrics table?
If so, others like CM are reading them, so you shouldn't change the type.
Also epoch seconds make doing time diffs easier.

@Rakshith606
Copy link
Contributor Author

these don't go to metrics table, these are only logged to hive.log

Copy link
Contributor

@cmunkey cmunkey left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@cmunkey cmunkey left a comment

Choose a reason for hiding this comment

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

Isn't there a way to have a custom mapper?

@cmunkey
Copy link
Contributor

cmunkey commented Jul 20, 2022

Maybe you can do the reverse of here:

https://stackoverflow.com/questions/69532856/serialize-java-time-duration-to-json-as-milliseconds-only-using-jackson

annotate the existing columns with custom mapper that maps as UTC time from epoch.

@cmunkey
Copy link
Contributor

cmunkey commented Jul 20, 2022

@JsonSerialize(using = UTCSerializer.class)
private Long dumpStartTime;

then

public class UTCSerializer extends JsonSerializer {

@Override
public void serialize(Long value, JsonGenerator gen, SerializerProvider serializers) throws IOException {
  LocalDateTime dateTime = LocalDateTime.ofEpochSecond(value, 0, ZoneOffset.UTC);
  // Might need to format here
  gen.writeString(dateTime.toString());
}

}

Then you just need to add annotation to all the State classes, and not change the type.

@cmunkey
Copy link
Contributor

cmunkey commented Jul 20, 2022

Add a unit test that serializes object and checks that the JSON string has a date and not epoch ms.

@cmunkey
Copy link
Contributor

cmunkey commented Jul 20, 2022

Since these are only logging messages, won't the log line already have the timestamp in readable format?
Won't it cause extra confusion if the log line and the message have a different times?
ReplState.log() does:

public void log(LogTag tag) {
try {
REPL_LOG.info("REPL::{}: {}", tag.name(), mapper.writeValueAsString(this));
} catch (Exception exception) {
REPL_LOG.error("Could not serialize REPL log: {}", exception.getMessage());
}
}
}

The object instance, BootstrapDumpBegin for instance, is created before the LOG.info(),
so could potentially have a different instance. Yes, converting from millis to seconds does
usually make them close, but log messages have:

2022-07-20 18:54:47,334 INFO

which is milliseconds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants