bbeaudreault commented on code in PR #4937:
URL: https://github.com/apache/hbase/pull/4937#discussion_r1082935124
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/OnlineLogRecord.java:
##########
@@ -36,10 +46,48 @@
@InterfaceStability.Evolving
final public class OnlineLogRecord extends LogEntry {
+ private static final Logger LOG =
LoggerFactory.getLogger(OnlineLogRecord.class.getName());
+
+ private static Optional<JsonElement> serializeCatchAll(Operation operation) {
+ try {
+ return Optional.of(JsonParser.parseString(operation.toJSON()));
+ } catch (Exception e) {
+ LOG.warn("Suppressing exception during OnlineLogRecord serialization
with operation {}",
+ operation, e);
+ return Optional.empty();
+ }
+ }
+
// used to convert object to pretty printed format
// used by toJsonPrettyPrint()
- private static final Gson GSON =
-
GsonUtil.createGson().setPrettyPrinting().registerTypeAdapter(OnlineLogRecord.class,
+ private static final Type OPERATION_LIST =
+ TypeToken.getParameterized(List.class, Operation.class).getType();
+ private static final Type OPERATION_MAYBE =
+ TypeToken.getParameterized(Optional.class, Operation.class).getType();
+ private static final Type OPERATION_LIST_MAYBE =
+ TypeToken.getParameterized(Optional.class,
OPERATION_LIST.getClass()).getType();
+ private static final JsonElement EMPTY_NODE =
JsonParser.parseString(HConstants.EMPTY_STRING);
+ private static final Gson GSON = GsonUtil.createGson().setPrettyPrinting()
+ .registerTypeAdapter(OPERATION_MAYBE.getClass(),
+ (JsonSerializer<
+ Optional<Operation>>) (operationMaybe, type, jsonSerializationContext)
-> operationMaybe
+ .map(operation ->
serializeCatchAll(operation).orElse(EMPTY_NODE)).orElse(EMPTY_NODE))
+ .registerTypeAdapter(OPERATION_LIST_MAYBE.getClass(), (JsonSerializer<
+ Optional<List<Operation>>>) (operationsMaybe, type,
jsonSerializationContext) -> {
+ if (!operationsMaybe.isPresent()) {
+ return EMPTY_NODE;
+ }
+ JsonObject jsonObj = new JsonObject();
+ final AtomicInteger i = new AtomicInteger(0);
+ for (Operation operation : operationsMaybe.get()) {
+ serializeCatchAll(operation).ifPresent(json -> {
+ jsonObj.add(String.valueOf(i), json);
+ i.incrementAndGet();
+ });
+ }
+ return jsonObj;
+ })
Review Comment:
this seems like a lot. maybe it's the only way, but maybe there's a
different way. Below we are registering a type adapter for
OnlineLogRecord.class. Currently we just call gson.toJsonTree then make a few
cleanups.
One question is whether we even need to fully serialize all the operation
stuff to json here. I'm not sure what this is used for exactly, but maybe we
don't have to include them? Otherwise, i wonder if there's a slightly simpler
way to represent these in the below type adapter.
I was imaginging you could even do something like this:
```
if (slowLogPayload.getScan().isPresent()) {
jsonObj.set("scan", slowLogPayload.getScan().toJSON());
}
etc
```
You're already checking for presence of these fields below in order to
remove the empties, so it might just be a matter of adding an `else` to each.
Not sure, not saying that's definitely better but something to think about.
I'm also surprised that there's not a built in type adapter for Optional and
List, in which case we would just need a type adapter for Operation which does
the toJSON() call.
--
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]
