[
https://issues.apache.org/jira/browse/OMID-239?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17692502#comment-17692502
]
ASF GitHub Bot commented on OMID-239:
-------------------------------------
stoty commented on code in PR #129:
URL: https://github.com/apache/phoenix-omid/pull/129#discussion_r1115256286
##########
transaction-client/src/test/resources/custom-omid-client-config.yml:
##########
@@ -0,0 +1,56 @@
+#
=====================================================================================================================
Review Comment:
let's call this tlstest-omid-client-config.yml or similar.
##########
hbase-common/pom.xml:
##########
@@ -92,6 +92,15 @@
<scope>test</scope>
</dependency>
+ <dependency>
Review Comment:
Shouldn't these be in the test scope ?
##########
hbase-common/src/main/java/org/apache/omid/tools/hbase/X509Util.java:
##########
@@ -0,0 +1,288 @@
+/*
+ * 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.omid.tools.hbase;
+
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslContextBuilder;
+import org.apache.hbase.thirdparty.com.google.common.collect.ObjectArrays;
Review Comment:
We shouldn't use HBase's thirdparty outside HBase.
IIRC we already depend on Phoenix's.
##########
transaction-client/pom.xml:
##########
@@ -35,6 +35,22 @@
<artifactId>omid-common</artifactId>
<version>${project.version}</version>
</dependency>
+
Review Comment:
nit move the compile dependencies near the beginning of the file, and the
test dependencies to the end.
##########
hbase-common/src/main/java/org/apache/omid/tools/hbase/X509Util.java:
##########
@@ -0,0 +1,288 @@
+/*
Review Comment:
I don't think that the TLS functionality belongs to hbase-common module.
We don't actually use it when connecting to HBase.
IMO this belongs in the omid-common module instead.
##########
tso-server/src/test/resources/custom-omid-server-config.yml:
##########
@@ -0,0 +1,182 @@
+#
+# Licensed 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.
+#
+
+#
=====================================================================================================================
+# Omid TSO Server Configuration (Default parameters)
+#
=====================================================================================================================
+
+#
---------------------------------------------------------------------------------------------------------------------
+# Basic configuration parameters
+#
---------------------------------------------------------------------------------------------------------------------
+
+# Network interface for TSO server communication. Uncomment the following line
to use a specific interface
+# networkIfaceName: eth0
+# If a network interface in the configuration, the TSO will attempt to guess
default network interface.
+# See org.apache.omid.tso.TSOServerConfig.getDefaultNetworkInterface for more
information.
+
+# Port reserved by the Status Oracle
+port: 54758
+# Wait strategy for the Disruptor processors in TSO pipeline. Options:
+# 1) HIGH_THROUGHPUT - [Default] Use this in production deployments for
maximum performance
+# 2) LOW_CPU - Use this option when testing or in deployments where saving CPU
cycles is more important than throughput
+waitStrategy: HIGH_THROUGHPUT
+# The number of elements reserved in the conflict map to perform conflict
resolution
+conflictMapSize: 100000000
+# The number of Commit Table writers that persist data concurrently to the
datastore. It has to be at least 2.
+numConcurrentCTWriters: 2
+# The size of the batch of operations that each Commit Table writes has. The
maximum number of operations that can be
+# batched in the system at a certain point in time is: numConcurrentCTWriters
* batchSizePerCTWriter
+batchSizePerCTWriter: 25
+# When this timeout expires, the contents of the batch are flushed to the
datastore
+batchPersistTimeoutInMs: 10
+# Timestamp generation strategy
+# INCREMENTAL - regular counter
+# WORLD_TIME - [Default] world time based counter
+timestampType: WORLD_TIME
+lowLatency: false
+# Default module configuration (No TSO High Availability & in-memory storage
for timestamp and commit tables)
+timestampStoreModule: !!org.apache.omid.tso.InMemoryTimestampStorageModule [ ]
+commitTableStoreModule: !!org.apache.omid.tso.InMemoryCommitTableStorageModule
[ ]
+leaseModule: !!org.apache.omid.tso.VoidLeaseManagementModule [ ]
+
+# Default stats/metrics configuration
+metrics: !!org.apache.omid.metrics.NullMetricsProvider [ ]
+
+monitorContext: false
+
+# TLS parameters
Review Comment:
While the names are self-explanatory, we may want to add a comment listing
all possible keys.
##########
hbase-common/src/test/java/org/apache/omid/tools/hbase/X509KeyType.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.omid.tools.hbase;
Review Comment:
Even though these are copied from HBase, they not used for HBase specific
features.
We should use some other package, like o.a.omid.tls (this goes for
everything in this package ofc)
BTW isn't o.a.omid.tools for the CLI tools used by Omid ?
##########
transaction-client/pom.xml:
##########
@@ -135,6 +151,16 @@
<artifactId>testng</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
Review Comment:
As I said elsewhere, this shouldn't depend on omid-hbase-common
##########
hbase-common/src/test/java/org/apache/omid/tools/hbase/TestX509Util.java:
##########
@@ -0,0 +1,388 @@
+/*
+ * 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.omid.tools.hbase;
+
+
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslContext;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
Review Comment:
I don't think these make sense in Omid.
##########
tso-server/src/test/resources/custom-omid-server-config.yml:
##########
@@ -0,0 +1,182 @@
+#
Review Comment:
rename this as well simlarly to the client config.
##########
hbase-common/src/test/java/org/apache/omid/tools/hbase/TestX509Util.java:
##########
@@ -0,0 +1,388 @@
+/*
+ * 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.omid.tools.hbase;
+
+
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslContext;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.testclassification.MiscTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.apache.zookeeper.common.KeyStoreFileType;
+import org.apache.zookeeper.common.X509Exception;
+import org.junit.After;
Review Comment:
Omid uses TestNG.
While we do want to move to Junit at some point, it should not be done on a
test-by-test basis.
##########
hbase-common/src/main/java/org/apache/omid/tools/hbase/X509Util.java:
##########
@@ -0,0 +1,288 @@
+/*
+ * 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.omid.tools.hbase;
+
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslContextBuilder;
+import org.apache.hbase.thirdparty.com.google.common.collect.ObjectArrays;
+import org.apache.yetus.audience.InterfaceAudience;
Review Comment:
Do we use InterfaceAudience in Omid ?
##########
hbase-common/src/test/java/org/apache/omid/tools/hbase/X509TestContext.java:
##########
@@ -0,0 +1,473 @@
+/*
+ * 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.omid.tools.hbase;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.zookeeper.common.KeyStoreFileType;
+import org.bouncycastle.asn1.x500.X500NameBuilder;
+import org.bouncycastle.asn1.x500.style.BCStyle;
+import org.bouncycastle.jce.provider.BouncyCastleProvider;
+import org.bouncycastle.operator.OperatorCreationException;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.security.KeyPair;
+import java.security.Security;
+import java.security.cert.X509Certificate;
+import java.util.Arrays;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * This class simplifies the creation of certificates and private keys for
SSL/TLS connections.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
Review Comment:
Did we take this directly from ZK or via HBase ?
If it's from HBase, then note that.
##########
transaction-client/pom.xml:
##########
@@ -135,6 +151,16 @@
<artifactId>testng</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.omid</groupId>
+ <artifactId>omid-hbase-common</artifactId>
+ <version>${project.version}</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
Review Comment:
Is this a new dependency ?
> OMID TLS support
> ----------------
>
> Key: OMID-239
> URL: https://issues.apache.org/jira/browse/OMID-239
> Project: Phoenix Omid
> Issue Type: Task
> Reporter: Richárd Antal
> Assignee: Richárd Antal
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
