[
https://issues.apache.org/jira/browse/GEODE-9360?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17371378#comment-17371378
]
ASF GitHub Bot commented on GEODE-9360:
---------------------------------------
pivotal-jbarrett commented on a change in pull request #823:
URL: https://github.com/apache/geode-native/pull/823#discussion_r659921472
##########
File path: netcore/LICENSE
##########
@@ -0,0 +1,201 @@
+ Apache License
Review comment:
This directory does not need to carry a separate license file. The root
of the repository has the appropriate license.
##########
File path: netcore/NetCore/CacheFactory.cs
##########
@@ -0,0 +1,129 @@
+/*
+* 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.
+*/
+using System;
+using System.Runtime.InteropServices;
+
+namespace Apache
+{
+ namespace Geode
+ {
+ namespace NetCore
+ {
+ public class CacheFactory : GeodeNativeObject, ICacheFactory
+ {
+ private string _version = String.Empty;
Review comment:
From this convention of `_` before a name it looks like you have derived
from
https://docs.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/coding-conventions.
Let's get this documented an enforced.
##########
File path: netcore/NetCore.Test/CacheFactoryUnitTest.cs
##########
@@ -0,0 +1,97 @@
+/*
+* 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.
+*/
+using System;
+using Apache.Geode.NetCore;
+using Xunit;
+
+namespace GemfireDotNetTest
+{
+ [Collection("Geode .net Core Collection")]
+ public class CacheFactoryUnitTests
+ {
Review comment:
In our contributing guide we should have a mention of the style guide
use for .NET Core sources. We never derived one for the old .NET Framework
C++/CLI sources because they were C++ based and loosely followed the C++ style
where it was even possible to apply it. No tooling completely supported C++/CLI
formatting so it is largely a mess. There is plenty of C# formatting tooling
out there including `clang-format`. Since `clang-format` is already a
requirement for C++ source formatting it make sense to extend it to C# sources
with the agreed apon style applied.
##########
File path: netcore/NOTICE
##########
@@ -0,0 +1,6 @@
+Apache Geode
Review comment:
Same as license, no need for notice here.
##########
File path: netcore/NetCore.Test/CacheFactoryUnitTest.cs
##########
@@ -0,0 +1,97 @@
+/*
+* 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.
+*/
+using System;
+using Apache.Geode.NetCore;
+using Xunit;
+
+namespace GemfireDotNetTest
+{
+ [Collection("Geode .net Core Collection")]
+ public class CacheFactoryUnitTests
+ {
+ [Fact]
+ public void TestCreateFactory()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ Assert.NotNull(cacheFactory);
+ }
+ }
+
+ [Fact]
+ public void TestCacheFactoryGetVersion()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ var version = cacheFactory.Version;
+ Assert.NotEqual(version, String.Empty);
+ }
+ }
+
+ [Fact]
+ public void TestCacheFactoryGetProductDescription()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ var description = cacheFactory.ProductDescription;
+ Assert.NotEqual(description, String.Empty);
+ }
+ }
+
+ [Fact]
+ public void TestCacheFactorySetPdxIgnoreUnreadFields()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ cacheFactory.PdxIgnoreUnreadFields = true;
+ cacheFactory.PdxIgnoreUnreadFields = false;
+ }
+ }
+
+ [Fact]
+ public void TestCacheFactorySetPdxReadSerialized()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ cacheFactory.PdxReadSerialized = true;
+ cacheFactory.PdxReadSerialized = false;
+ }
+ }
+
+ [Fact]
+ public void TestCacheFactoryCreateCache()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ using (var cache = cacheFactory.CreateCache()) // lgtm[cs /
useless - assignment - to - local]
+ {
+ ;
+ }
+ }
+ }
+
+ [Fact]
+ public void TestCacheFactorySetProperty()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ cacheFactory.SetProperty("log-level", "none")
+ .SetProperty("log-file", "geode_native.log");
+ }
+ }
+ }
+}
Review comment:
Always end with a new line.
##########
File path: netcore/geode-dotnet-core.sln
##########
@@ -0,0 +1,31 @@
+
Review comment:
CMake can generate VS project files if that is your preferred generator
so why are we checking them in?
##########
File path: netcore/NetCore/CacheFactory.cs
##########
@@ -0,0 +1,129 @@
+/*
+* 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.
+*/
+using System;
+using System.Runtime.InteropServices;
+
+namespace Apache
+{
+ namespace Geode
+ {
+ namespace NetCore
+ {
Review comment:
Can we derive a formatting that doesn't indent for namespaces or can C#
support single line fully qualified namespaces?
##########
File path: netcore/NetCore/IRegionService.cs
##########
@@ -0,0 +1,35 @@
+/*
+* 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.
+*/
+using System;
+
+namespace Apache
+{
+ namespace Geode
+ {
+ namespace NetCore
+ {
+ public interface IRegionService
+ {
+// IPdxInstanceFactory CreatePdxInstanceFactory(String
className);
Review comment:
Remove commented out source.
##########
File path: netcore/NetCore.Test/CacheFactoryUnitTest.cs
##########
@@ -0,0 +1,97 @@
+/*
+* 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.
+*/
+using System;
+using Apache.Geode.NetCore;
Review comment:
Why does the .NET Core library have the "framework" name in the
namespace. We didn't do this with any of the other languages/frameworks. In the
others the namespace for most client cases is [org.]apache.geode.client. I
think this should be changed to `Apache.Geode.Client`. Yes it collides with the
.NET Framework version but these can't be used together in the same processes
space so there isn't any confusion or issues with that.
##########
File path: netcore/build-geode-native-netcore.sh
##########
@@ -0,0 +1,54 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
Review comment:
What is the purpose of this helper script to build? Shouldn't this be
tied into the main CMake build process?
##########
File path: netcore/utility/SimpleSecurityManager.java
##########
@@ -0,0 +1,79 @@
+/*
+ * 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 javaobject;
+
+import org.apache.geode.security.AuthenticationFailedException;
+import org.apache.geode.security.SecurityManager;
+
+import java.util.Properties;
+
+import javaobject.UserPasswordAuthInit;
+import javaobject.UsernamePrincipal;
+
+/**
+ * This Security manager only Authenticates - and allows any operations.
+ */
+public class SimpleSecurityManager implements SecurityManager {
Review comment:
Don't we have a version of this already in the existing repo? We
shouldn't need more than one fake `SecurityManager` for testing.
##########
File path: netcore/startserver.ps1
##########
@@ -0,0 +1,65 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
Review comment:
This looks like part of a test framework maybe, why is it in the root of
the subproject?
##########
File path: netcore/utility/UserPasswordAuthInit.java
##########
@@ -0,0 +1,81 @@
+/*
Review comment:
Same as with `SecurityManager` implementation. Also, the use of
`AuthInit` on the server side is deprecated in favor of `SecurityManager` so it
really strikes me as unnecessary for a new client to validate with a deprecated
feature of the server.
##########
File path: netcore/utility/CMakeLists.txt
##########
@@ -0,0 +1,35 @@
+# 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.
+cmake_minimum_required (VERSION 3.4)
Review comment:
3.4 seems very old and is probably lacking some of the .NET features we
need to properly build this. This shouldn't be set here since the root CMake
will be setting it.
##########
File path: netcore/NetCore.Test/CacheFactoryUnitTest.cs
##########
@@ -0,0 +1,97 @@
+/*
+* 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.
+*/
+using System;
+using Apache.Geode.NetCore;
Review comment:
It will be very complicated to change after the first non-beta release
too, so sooner than later on this one.
##########
File path: netcore/NetCore.Test/CacheFactoryUnitTest.cs
##########
@@ -0,0 +1,97 @@
+/*
+* 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.
+*/
+using System;
+using Apache.Geode.NetCore;
+using Xunit;
+
+namespace GemfireDotNetTest
+{
+ [Collection("Geode .net Core Collection")]
+ public class CacheFactoryUnitTests
+ {
Review comment:
I think adding in C# will have less urgency given that 99% of the work
will likely be on Windows anyway. I also thing the variants in formatting are
much fewer than C++ given the relative age ad popularity C++ has of C#.
##########
File path: netcore/NetCore.Test/CacheFactoryUnitTest.cs
##########
@@ -0,0 +1,97 @@
+/*
+* 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.
+*/
+using System;
+using Apache.Geode.NetCore;
+using Xunit;
+
+namespace GemfireDotNetTest
+{
+ [Collection("Geode .net Core Collection")]
+ public class CacheFactoryUnitTests
+ {
Review comment:
I think adding in C# will have less urgency given that 99% of the work
will likely be on Windows anyway. I also thing the variants in formatting are
much fewer than C++ given the relative age and popularity C++ has of C#.
##########
File path: netcore/NetCore.Test/CacheFactoryUnitTest.cs
##########
@@ -0,0 +1,97 @@
+/*
+* 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.
+*/
+using System;
+using Apache.Geode.NetCore;
+using Xunit;
+
+namespace GemfireDotNetTest
+{
+ [Collection("Geode .net Core Collection")]
+ public class CacheFactoryUnitTests
+ {
+ [Fact]
+ public void TestCreateFactory()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ Assert.NotNull(cacheFactory);
+ }
+ }
+
+ [Fact]
+ public void TestCacheFactoryGetVersion()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ var version = cacheFactory.Version;
+ Assert.NotEqual(version, String.Empty);
+ }
+ }
+
+ [Fact]
+ public void TestCacheFactoryGetProductDescription()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ var description = cacheFactory.ProductDescription;
+ Assert.NotEqual(description, String.Empty);
+ }
+ }
+
+ [Fact]
+ public void TestCacheFactorySetPdxIgnoreUnreadFields()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ cacheFactory.PdxIgnoreUnreadFields = true;
+ cacheFactory.PdxIgnoreUnreadFields = false;
+ }
+ }
+
+ [Fact]
+ public void TestCacheFactorySetPdxReadSerialized()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ cacheFactory.PdxReadSerialized = true;
+ cacheFactory.PdxReadSerialized = false;
+ }
+ }
+
+ [Fact]
+ public void TestCacheFactoryCreateCache()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ using (var cache = cacheFactory.CreateCache()) // lgtm[cs /
useless - assignment - to - local]
+ {
+ ;
+ }
+ }
+ }
+
+ [Fact]
+ public void TestCacheFactorySetProperty()
+ {
+ using (var cacheFactory = CacheFactory.Create())
+ {
+ cacheFactory.SetProperty("log-level", "none")
+ .SetProperty("log-file", "geode_native.log");
+ }
+ }
+ }
+}
Review comment:
clang-format will enforce for sure. Kind of surprised the VS formatter
didn't
##########
File path: netcore/geode-dotnet-core.sln
##########
@@ -0,0 +1,31 @@
+
Review comment:
That seems odd to me since most of the work to support C# and .NET in
CMake is for .NET Core but perhaps with an emphasis on not being stuck with VS
build as the build toolkit. Since I should be able to build .NET Core on my Mac
I shouldn't need VS Studio and build tools to do so right? Something we should
certainly improve on because checking in any project files has never gone well
in all of the history of software development. ;)
--
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: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Initial revision of .net core support
> -------------------------------------
>
> Key: GEODE-9360
> URL: https://issues.apache.org/jira/browse/GEODE-9360
> Project: Geode
> Issue Type: Improvement
> Components: native client
> Reporter: Blake Bender
> Priority: Major
> Labels: pull-request-available
>
> As a .net core developer, I would like to be able to access the geode-native
> API. To facilitate this, we need to implement a minimal set of C# classes
> that use p/invoke to access geode-native via C bindings (GEODE-9358).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
