[
https://issues.apache.org/jira/browse/HIVE-26363?focusedWorklogId=802327&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-802327
]
ASF GitHub Bot logged work on HIVE-26363:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 22/Aug/22 03:19
Start Date: 22/Aug/22 03:19
Worklog Time Spent: 10m
Work Description: pudidic commented on code in PR #3541:
URL: https://github.com/apache/hive/pull/3541#discussion_r950978148
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/repl/TestReplStateLogTimeFormat.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.parse.repl;
+
+import org.apache.hadoop.hive.ql.parse.repl.load.log.state.DataCopyEnd;
+import org.apache.hadoop.hive.ql.exec.repl.util.ReplUtils;
+import org.reflections.Reflections;
+import org.junit.Test;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+
+import java.lang.reflect.Field;
+import java.time.Instant;
+import java.util.HashSet;
+import java.util.Objects;
+import java.util.Set;
+
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+
+public class TestReplStateLogTimeFormat {
+ private static final long randomDate = 1659367078L;
+ private static final String UTCString =
Instant.ofEpochSecond(randomDate).toString();
+ private static final Set<Class<? extends ReplState>>
DUMP_LOG_EXEMPTED_CLASSES;
+ private static final Set<Class<? extends ReplState>>
LOAD_LOG_EXEMPTED_CLASSES;
+
+ static {
+ // Add classes which don't have a time field or time fields which need not
be serialized to UTC format.
+ DUMP_LOG_EXEMPTED_CLASSES = new HashSet<>();
+
+ LOAD_LOG_EXEMPTED_CLASSES = new HashSet<>();
+ LOAD_LOG_EXEMPTED_CLASSES.add(DataCopyEnd.class);
+ }
+
+ private void verifyAnnotationAndSetEpoch(ReplState replState) throws
Exception {
+ boolean isAnnotationSet = false;
+ for (Field field : replState.getClass().getDeclaredFields()) {
+ if (Objects.nonNull(field.getAnnotation(JsonSerialize.class))) {
+ if
(ReplUtils.TimeSerializer.class.equals(field.getAnnotation(JsonSerialize.class).using()))
{
+ field.setAccessible(true);
+ field.set(replState, randomDate);
+ isAnnotationSet = true;
+ }
+ }
+ }
+ assertTrue(
+ String.format("Class %s has a time field which is not annotated
with " +
+ "@JsonSerialize(using =
ReplUtils.TimeSerializer.class) " +
+ "Please annotate the time field with it or add it
to appropriate exempted set above",
+ replState.getClass().getName()),
+ isAnnotationSet
+ );
+ }
+
+ private void verifyUTCString(ReplState replState) throws Exception {
+ ObjectMapper objectMapper = new ObjectMapper();
+ String json = objectMapper.writeValueAsString(replState);
+ assertTrue(
+ String.format("Expected UTC string %s not found in serialized
representation of %s",
+ UTCString, replState.getClass().getName()),
+ json.contains(UTCString)
+ );
+ }
+
+
+ private void testTimeFormat(String packagePath, Set<Class<? extends
ReplState>> EXEMPTED_CLASS) throws Exception {
+ Set<Class<? extends ReplState>> replStateLogClasses
+ = new Reflections(packagePath).getSubTypesOf(ReplState.class);
+ for (Class<? extends ReplState> cls : replStateLogClasses) {
+ if (EXEMPTED_CLASS.contains(cls)) {
+ continue;
+ }
+ ReplState replState = mock(cls);
+ verifyAnnotationAndSetEpoch(replState);
+ verifyUTCString(replState);
+ }
+ }
+
+ @Test
+ public void test() throws Exception {
Review Comment:
How about having a more specific name to reveal more about its intention?
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/repl/TestReplStateLogTimeFormat.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.parse.repl;
+
+import org.apache.hadoop.hive.ql.parse.repl.load.log.state.DataCopyEnd;
+import org.apache.hadoop.hive.ql.exec.repl.util.ReplUtils;
+import org.reflections.Reflections;
+import org.junit.Test;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+
+import java.lang.reflect.Field;
+import java.time.Instant;
+import java.util.HashSet;
+import java.util.Objects;
+import java.util.Set;
+
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+
+public class TestReplStateLogTimeFormat {
+ private static final long randomDate = 1659367078L;
+ private static final String UTCString =
Instant.ofEpochSecond(randomDate).toString();
+ private static final Set<Class<? extends ReplState>>
DUMP_LOG_EXEMPTED_CLASSES;
+ private static final Set<Class<? extends ReplState>>
LOAD_LOG_EXEMPTED_CLASSES;
+
+ static {
+ // Add classes which don't have a time field or time fields which need not
be serialized to UTC format.
+ DUMP_LOG_EXEMPTED_CLASSES = new HashSet<>();
+
+ LOAD_LOG_EXEMPTED_CLASSES = new HashSet<>();
+ LOAD_LOG_EXEMPTED_CLASSES.add(DataCopyEnd.class);
+ }
+
+ private void verifyAnnotationAndSetEpoch(ReplState replState) throws
Exception {
+ boolean isAnnotationSet = false;
+ for (Field field : replState.getClass().getDeclaredFields()) {
+ if (Objects.nonNull(field.getAnnotation(JsonSerialize.class))) {
+ if
(ReplUtils.TimeSerializer.class.equals(field.getAnnotation(JsonSerialize.class).using()))
{
+ field.setAccessible(true);
+ field.set(replState, randomDate);
+ isAnnotationSet = true;
+ }
+ }
+ }
+ assertTrue(
+ String.format("Class %s has a time field which is not annotated
with " +
Review Comment:
The current Hive code base usually indents with 2 or 4 whitespaces for new
lines with open brackets. There are 8 ones, but they are rare.
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/repl/TestReplStateLogTimeFormat.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.parse.repl;
+
+import org.apache.hadoop.hive.ql.parse.repl.load.log.state.DataCopyEnd;
+import org.apache.hadoop.hive.ql.exec.repl.util.ReplUtils;
+import org.reflections.Reflections;
+import org.junit.Test;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+
+import java.lang.reflect.Field;
+import java.time.Instant;
+import java.util.HashSet;
+import java.util.Objects;
+import java.util.Set;
+
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+
+public class TestReplStateLogTimeFormat {
+ private static final long randomDate = 1659367078L;
+ private static final String UTCString =
Instant.ofEpochSecond(randomDate).toString();
+ private static final Set<Class<? extends ReplState>>
DUMP_LOG_EXEMPTED_CLASSES;
+ private static final Set<Class<? extends ReplState>>
LOAD_LOG_EXEMPTED_CLASSES;
+
+ static {
+ // Add classes which don't have a time field or time fields which need not
be serialized to UTC format.
+ DUMP_LOG_EXEMPTED_CLASSES = new HashSet<>();
+
+ LOAD_LOG_EXEMPTED_CLASSES = new HashSet<>();
+ LOAD_LOG_EXEMPTED_CLASSES.add(DataCopyEnd.class);
+ }
+
+ private void verifyAnnotationAndSetEpoch(ReplState replState) throws
Exception {
+ boolean isAnnotationSet = false;
+ for (Field field : replState.getClass().getDeclaredFields()) {
+ if (Objects.nonNull(field.getAnnotation(JsonSerialize.class))) {
+ if
(ReplUtils.TimeSerializer.class.equals(field.getAnnotation(JsonSerialize.class).using()))
{
+ field.setAccessible(true);
+ field.set(replState, randomDate);
+ isAnnotationSet = true;
+ }
+ }
+ }
+ assertTrue(
+ String.format("Class %s has a time field which is not annotated
with " +
+ "@JsonSerialize(using =
ReplUtils.TimeSerializer.class) " +
+ "Please annotate the time field with it or add it
to appropriate exempted set above",
+ replState.getClass().getName()),
+ isAnnotationSet
+ );
+ }
+
+ private void verifyUTCString(ReplState replState) throws Exception {
+ ObjectMapper objectMapper = new ObjectMapper();
+ String json = objectMapper.writeValueAsString(replState);
+ assertTrue(
+ String.format("Expected UTC string %s not found in serialized
representation of %s",
+ UTCString, replState.getClass().getName()),
+ json.contains(UTCString)
+ );
+ }
+
+
+ private void testTimeFormat(String packagePath, Set<Class<? extends
ReplState>> EXEMPTED_CLASS) throws Exception {
+ Set<Class<? extends ReplState>> replStateLogClasses
+ = new Reflections(packagePath).getSubTypesOf(ReplState.class);
+ for (Class<? extends ReplState> cls : replStateLogClasses) {
+ if (EXEMPTED_CLASS.contains(cls)) {
+ continue;
+ }
+ ReplState replState = mock(cls);
+ verifyAnnotationAndSetEpoch(replState);
+ verifyUTCString(replState);
+ }
+ }
+
+ @Test
+ public void test() throws Exception {
+ testTimeFormat("org.apache.hadoop.hive.ql.parse.repl.dump.log.state",
DUMP_LOG_EXEMPTED_CLASSES);
+ testTimeFormat("org.apache.hadoop.hive.ql.parse.repl.load.log.state",
LOAD_LOG_EXEMPTED_CLASSES);
+ }
+
+ @Test(expected = AssertionError.class)
+ public void testClassWithoutAnnotation() throws Exception {
+ ReplState replStateClassWithoutAnnotation = new ReplState() {
+ private Long StartTime;
+ };
+ verifyAnnotationAndSetEpoch(replStateClassWithoutAnnotation);
+ }
+
+}
Review Comment:
We usually end a file with a new line.
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/repl/TestReplStateLogTimeFormat.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.parse.repl;
+
+import org.apache.hadoop.hive.ql.parse.repl.load.log.state.DataCopyEnd;
+import org.apache.hadoop.hive.ql.exec.repl.util.ReplUtils;
+import org.reflections.Reflections;
+import org.junit.Test;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+
+import java.lang.reflect.Field;
+import java.time.Instant;
+import java.util.HashSet;
+import java.util.Objects;
+import java.util.Set;
+
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+
+public class TestReplStateLogTimeFormat {
+ private static final long randomDate = 1659367078L;
+ private static final String UTCString =
Instant.ofEpochSecond(randomDate).toString();
+ private static final Set<Class<? extends ReplState>>
DUMP_LOG_EXEMPTED_CLASSES;
+ private static final Set<Class<? extends ReplState>>
LOAD_LOG_EXEMPTED_CLASSES;
+
+ static {
+ // Add classes which don't have a time field or time fields which need not
be serialized to UTC format.
+ DUMP_LOG_EXEMPTED_CLASSES = new HashSet<>();
+
+ LOAD_LOG_EXEMPTED_CLASSES = new HashSet<>();
+ LOAD_LOG_EXEMPTED_CLASSES.add(DataCopyEnd.class);
+ }
+
+ private void verifyAnnotationAndSetEpoch(ReplState replState) throws
Exception {
+ boolean isAnnotationSet = false;
+ for (Field field : replState.getClass().getDeclaredFields()) {
+ if (Objects.nonNull(field.getAnnotation(JsonSerialize.class))) {
+ if
(ReplUtils.TimeSerializer.class.equals(field.getAnnotation(JsonSerialize.class).using()))
{
+ field.setAccessible(true);
+ field.set(replState, randomDate);
+ isAnnotationSet = true;
+ }
+ }
+ }
+ assertTrue(
+ String.format("Class %s has a time field which is not annotated
with " +
+ "@JsonSerialize(using =
ReplUtils.TimeSerializer.class) " +
+ "Please annotate the time field with it or add it
to appropriate exempted set above",
+ replState.getClass().getName()),
+ isAnnotationSet
+ );
+ }
+
+ private void verifyUTCString(ReplState replState) throws Exception {
+ ObjectMapper objectMapper = new ObjectMapper();
+ String json = objectMapper.writeValueAsString(replState);
+ assertTrue(
+ String.format("Expected UTC string %s not found in serialized
representation of %s",
+ UTCString, replState.getClass().getName()),
+ json.contains(UTCString)
+ );
+ }
+
+
+ private void testTimeFormat(String packagePath, Set<Class<? extends
ReplState>> EXEMPTED_CLASS) throws Exception {
Review Comment:
How about having 'check' or 'verify' for its prefix? Because 'test' prefix
is usually used for JUnit test methods with the Test annotation.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/repl/dump/log/state/AtlasDumpBegin.java:
##########
@@ -17,6 +17,8 @@
*/
package org.apache.hadoop.hive.ql.parse.repl.dump.log.state;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
Review Comment:
How about grouping with the line 25 `import
com.fasterxml.jackson.annotation.JsonProperty;`? A same comment applies for
other files, too.
Issue Time Tracking
-------------------
Worklog Id: (was: 802327)
Time Spent: 1h 50m (was: 1h 40m)
> Time logged during repldump and replload per table is not in readable format
> ----------------------------------------------------------------------------
>
> Key: HIVE-26363
> URL: https://issues.apache.org/jira/browse/HIVE-26363
> Project: Hive
> Issue Type: Improvement
> Components: HiveServer2, repl
> Affects Versions: 4.0.0
> Reporter: Imran
> Assignee: Rakshith C
> Priority: Minor
> Labels: pull-request-available
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
> During replDump and replLoad we capture time take for each activity in
> hive.log file. This is captured in milliseconds which becomes difficult to
> read during debug activity, this ticket is raised to change the time logged
> in hive.log in UTC format.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
