You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/01/14 13:40:47 UTC
[GitHub] [ignite-extensions] ololo3000 opened a new pull request #33: IGNITE-13992 Migrates Spring Transactions integration to ignite-extensions.
ololo3000 opened a new pull request #33:
URL: https://github.com/apache/ignite-extensions/pull/33
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #33: IGNITE-13992 Migrates Spring Transactions integration to ignite-extensions.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #33:
URL: https://github.com/apache/ignite-extensions/pull/33#discussion_r570823720
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/internal/transactions/proxy/ClientTransactionProxy.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.internal.transactions.proxy;
+
+import org.apache.ignite.client.ClientTransaction;
+import org.apache.ignite.internal.util.typedef.internal.S;
+
+/**
+ * Represents {@link TransactionProxy} implementation that uses {@link ClientTransaction} to perform transaction
+ * operations.
+ */
+public class ClientTransactionProxy implements TransactionProxy {
Review comment:
My motivation to put the tx proxy classes in spring-data-commons is to make it possible to reuse all the proxy classes.
In the future, I would like to rename spring-data-commons to spring-commons.
I think it is useful to keep all of the Spring utility classes in one module - for example, the IgniteCacheProxy, which is now stored in spring-data-commons, will be reused in the Spring Cache thin client integration. But earlier I claimed that IgniteCacheProxy is only needed for Spring Data. The same can be with the tx proxy.
Does this make sense?
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/internal/transactions/proxy/ClientTransactionProxy.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.internal.transactions.proxy;
+
+import org.apache.ignite.client.ClientTransaction;
+import org.apache.ignite.internal.util.typedef.internal.S;
+
+/**
+ * Represents {@link TransactionProxy} implementation that uses {@link ClientTransaction} to perform transaction
+ * operations.
+ */
+public class ClientTransactionProxy implements TransactionProxy {
Review comment:
I moved proxy classes back to tx-ext module.
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/internal/transactions/proxy/ClientTransactionProxy.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.internal.transactions.proxy;
+
+import org.apache.ignite.client.ClientTransaction;
+import org.apache.ignite.internal.util.typedef.internal.S;
+
+/**
+ * Represents {@link TransactionProxy} implementation that uses {@link ClientTransaction} to perform transaction
+ * operations.
+ */
+public class ClientTransactionProxy implements TransactionProxy {
Review comment:
I moved proxy classes back to tx-ext module. New TC build - https://ci.ignite.apache.org/buildConfiguration/IgniteExtensions_Tests_RunAllTests/5857838
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] alex-plekhanov commented on a change in pull request #33: IGNITE-13992 Migrates Spring Transactions integration to ignite-extensions.
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #33:
URL: https://github.com/apache/ignite-extensions/pull/33#discussion_r570830307
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/internal/transactions/proxy/ClientTransactionProxy.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.internal.transactions.proxy;
+
+import org.apache.ignite.client.ClientTransaction;
+import org.apache.ignite.internal.util.typedef.internal.S;
+
+/**
+ * Represents {@link TransactionProxy} implementation that uses {@link ClientTransaction} to perform transaction
+ * operations.
+ */
+public class ClientTransactionProxy implements TransactionProxy {
Review comment:
Spring data proxies are used by 3 modules, so it makes sense to move them into the common module to reduce duplication. Spring-tx proxies only used by one. If in the future there will be new modules that will require spring-tx proxies, we can move these classes to the common module, but now I don't know modules except spring-tx that require spring-tx proxies. Did I miss something?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #33: IGNITE-13992 Migrates Spring Transactions integration to ignite-extensions.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #33:
URL: https://github.com/apache/ignite-extensions/pull/33#discussion_r570823720
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/internal/transactions/proxy/ClientTransactionProxy.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.internal.transactions.proxy;
+
+import org.apache.ignite.client.ClientTransaction;
+import org.apache.ignite.internal.util.typedef.internal.S;
+
+/**
+ * Represents {@link TransactionProxy} implementation that uses {@link ClientTransaction} to perform transaction
+ * operations.
+ */
+public class ClientTransactionProxy implements TransactionProxy {
Review comment:
My motivation to put the tx proxy classes in spring-data-commons is to make it possible to reuse all the proxy classes.
In the future, I would like to rename spring-data-commons to spring-commons.
I think it is useful to keep all of the Spring utility classes in one module - for example, the IgniteCacheProxy, which is now stored in spring-data-commons, will be reused in the Spring Cache thin client integration. But earlier I claimed that IgniteCacheProxy is only needed for Spring Data. The same can be with the tx proxy.
Does this make sense?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] asfgit closed pull request #33: IGNITE-13992 Migrates Spring Transactions integration to ignite-extensions.
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #33:
URL: https://github.com/apache/ignite-extensions/pull/33
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] alex-plekhanov commented on a change in pull request #33: IGNITE-13992 Migrates Spring Transactions integration to ignite-extensions.
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #33:
URL: https://github.com/apache/ignite-extensions/pull/33#discussion_r570808096
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/internal/transactions/proxy/ClientTransactionProxy.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.internal.transactions.proxy;
+
+import org.apache.ignite.client.ClientTransaction;
+import org.apache.ignite.internal.util.typedef.internal.S;
+
+/**
+ * Represents {@link TransactionProxy} implementation that uses {@link ClientTransaction} to perform transaction
+ * operations.
+ */
+public class ClientTransactionProxy implements TransactionProxy {
Review comment:
As far as I understand transaction proxies don't use anyhow cache data proxies and cache data proxies don't use anyhow transactions proxies. Transactions proxies required only by one module spring-tx, this module can be used without spring data, so I think we should move back these classes to spring-tx module.
Moreover, there is no dependency from spring-tx to spring-data-commons module.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] alex-plekhanov commented on a change in pull request #33: IGNITE-13992 Migrates Spring Transactions integration to ignite-extensions.
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #33:
URL: https://github.com/apache/ignite-extensions/pull/33#discussion_r570808096
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/internal/transactions/proxy/ClientTransactionProxy.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.internal.transactions.proxy;
+
+import org.apache.ignite.client.ClientTransaction;
+import org.apache.ignite.internal.util.typedef.internal.S;
+
+/**
+ * Represents {@link TransactionProxy} implementation that uses {@link ClientTransaction} to perform transaction
+ * operations.
+ */
+public class ClientTransactionProxy implements TransactionProxy {
Review comment:
As far as I understand transaction proxies don't use anyhow cache data proxies and cache data proxies don't use anyhow transactions proxies. Transactions proxies required only by one module spring-tx, this module can be used without spring data, so I think we should move back these classes to spring-tx module.
Moreover, there is no dependency from spring-tx to spring-data-commons module.
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/internal/transactions/proxy/ClientTransactionProxy.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.internal.transactions.proxy;
+
+import org.apache.ignite.client.ClientTransaction;
+import org.apache.ignite.internal.util.typedef.internal.S;
+
+/**
+ * Represents {@link TransactionProxy} implementation that uses {@link ClientTransaction} to perform transaction
+ * operations.
+ */
+public class ClientTransactionProxy implements TransactionProxy {
Review comment:
Spring data proxies are used by 3 modules, so it makes sense to move them into the common module to reduce duplication. Spring-tx proxies only used by one. If in the future there will be new modules that will require spring-tx proxies, we can move these classes to the common module, but now I don't know modules except spring-tx that require spring-tx proxies. Did I miss something?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org