ascherbakoff commented on code in PR #896: URL: https://github.com/apache/ignite-3/pull/896#discussion_r905974069
########## modules/core/src/test/java/org/apache/ignite/hlc/HybridClockTestUtils.java: ########## @@ -0,0 +1,47 @@ +/* + * 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.ignite.hlc; + +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; + +import java.time.Clock; +import java.time.Instant; +import org.mockito.MockedStatic; + +/** + * Utils for a Hybrid Logical Clock testing. + */ +public class HybridClockTestUtils { + /** + * Creates a mocked system clock. + * + * @param expected Expected value which returns by system clock. + * @return The mocked clock. + */ + public static MockedStatic<Clock> mockToEpochMilli(long expected) { Review Comment: Here is the citation from the internet on statick mocks: "Generally speaking, some might say that when writing clean code, we shouldn't need to mock static classes. This could typically hint at a design issue or code smell in the application. Why? First, a class depending on a static method has tight coupling, and second, it nearly always leads to code that is difficult to test. Ideally, a class should not be responsible for obtaining its dependencies, and if possible, they should be externally injected. So, it's always worth investigating if we can refactor our code to make it more testable." I suggest to take it into a consideration. ########## modules/core/src/main/java/org/apache/ignite/hlc/HybridTimestamp.java: ########## @@ -0,0 +1,111 @@ +/* + * 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.ignite.hlc; + +import org.apache.ignite.internal.tostring.S; + +/** + * A hybrid timestamp that combines physical clock and logical clock. + */ +public class HybridTimestamp implements Comparable<HybridTimestamp> { + /** Physical clock. */ + private final long physical; + + /** Logical clock. */ + private final int logical; + + /** + * The constructor. + * + * @param physical The physical time. + * @param logical The logical time. + */ + public HybridTimestamp(long physical, int logical) { + this.physical = physical; + this.logical = logical; + } + + /** + * Compares hybrid timestamps. + * + * @param times Times for comparing. + * @return The highest hybrid timestamp. + */ + public static HybridTimestamp max(HybridTimestamp... times) { + HybridTimestamp maxTime = times[0]; + + for (int i = 1; i < times.length; i++) { + maxTime = maxTime.compareTo(times[i]) > 0 ? maxTime : times[i]; Review Comment: The assignment is needed only for new max. ########## modules/core/src/main/java/org/apache/ignite/hlc/HybridTimestamp.java: ########## @@ -0,0 +1,111 @@ +/* + * 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.ignite.hlc; + +import org.apache.ignite.internal.tostring.S; + +/** + * A hybrid timestamp that combines physical clock and logical clock. + */ +public class HybridTimestamp implements Comparable<HybridTimestamp> { + /** Physical clock. */ + private final long physical; + + /** Logical clock. */ + private final int logical; + + /** + * The constructor. + * + * @param physical The physical time. + * @param logical The logical time. + */ + public HybridTimestamp(long physical, int logical) { + this.physical = physical; + this.logical = logical; + } + + /** + * Compares hybrid timestamps. + * + * @param times Times for comparing. + * @return The highest hybrid timestamp. + */ + public static HybridTimestamp max(HybridTimestamp... times) { + HybridTimestamp maxTime = times[0]; + + for (int i = 1; i < times.length; i++) { + maxTime = maxTime.compareTo(times[i]) > 0 ? maxTime : times[i]; + } + + return maxTime; + } + + /** + * Returns a physical component. + * + * @return The physical component. + */ + public long getPhysical() { + return physical; + } + + /** + * Returns a logical component. + * + * @return The logical component. + */ + public int getLogical() { + return logical; + } + + /** + * Returns a new hybrid timestamp with incremented logical component. + * + * @return The hybrid timestamp. + */ + public HybridTimestamp addTicks(int ticks) { + return new HybridTimestamp(physical, this.logical + ticks); Review Comment: Creating an object on tick is not good ########## modules/core/src/main/java/org/apache/ignite/hlc/HybridTimestamp.java: ########## @@ -0,0 +1,111 @@ +/* + * 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.ignite.hlc; + +import org.apache.ignite.internal.tostring.S; + +/** + * A hybrid timestamp that combines physical clock and logical clock. + */ +public class HybridTimestamp implements Comparable<HybridTimestamp> { + /** Physical clock. */ + private final long physical; + + /** Logical clock. */ + private final int logical; + + /** + * The constructor. + * + * @param physical The physical time. + * @param logical The logical time. + */ + public HybridTimestamp(long physical, int logical) { + this.physical = physical; + this.logical = logical; + } + + /** + * Compares hybrid timestamps. + * + * @param times Times for comparing. + * @return The highest hybrid timestamp. + */ + public static HybridTimestamp max(HybridTimestamp... times) { Review Comment: Why is this method declared _public_ ? ########## modules/core/src/main/java/org/apache/ignite/hlc/HybridTimestamp.java: ########## @@ -0,0 +1,111 @@ +/* + * 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.ignite.hlc; + +import org.apache.ignite.internal.tostring.S; + +/** + * A hybrid timestamp that combines physical clock and logical clock. + */ +public class HybridTimestamp implements Comparable<HybridTimestamp> { Review Comment: The implementation seems correct, but not optimal. It produces new objects and does unnecessary operations. I suggest to look at https://github.com/eventreducer/hlc/blob/master/src/main/java/org/eventreducer/hlc/HybridTimestamp.java as a good reference. It also has a nice abstaction of physical time provider. ########## modules/core/src/main/java/org/apache/ignite/hlc/HybridClock.java: ########## @@ -0,0 +1,75 @@ +/* + * 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.ignite.hlc; + +import java.time.Clock; +import org.apache.ignite.internal.tostring.S; + +/** + * A Hybrid Logical Clock. + */ +public class HybridClock { + /** Latest timestamp. */ + private HybridTimestamp latestTime; + + /** + * The constructor which initializes the latest time to current time by system clock. + */ + public HybridClock() { + this.latestTime = new HybridTimestamp(Clock.systemUTC().instant().toEpochMilli(), 0); Review Comment: Default value for logicalTime is 0, according to paper -- 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]
