You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@jena.apache.org by GitBox <gi...@apache.org> on 2020/11/26 21:50:01 UTC
[GitHub] [jena] strangepleasures opened a new pull request #872: JENA-2004 Implement functionality currently provided by Txn class directly in Transactional. Deprecate Txn class.
strangepleasures opened a new pull request #872:
URL: https://github.com/apache/jena/pull/872
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs closed pull request #872: JENA-2004 Implement functionality currently provided by Txn class directly in Transactional. Deprecate Txn class.
Posted by GitBox <gi...@apache.org>.
afs closed pull request #872:
URL: https://github.com/apache/jena/pull/872
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #872: JENA-2004 Implement functionality currently provided by Txn class directly in Transactional. Deprecate Txn class.
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #872:
URL: https://github.com/apache/jena/pull/872#issuecomment-735378555
From a style point of view?
From a technology POV, I don't think the circularity is a problem - it worked for me in a quick experiment - neither of these touch Jena very much and don't have any state or constants. They are in the same codebase. Sometimes, system initialization needs to be adjusted because only certain points call `static { JenaSystem.init(); }` - generally, this can be added anywhere. Have you encountered a specific problem?
Style-wise - the default methods could go on `Dataset`, not `Transactional`, and leave `Transactional` as the core transaction abstraction without built-in helpers.
Rather than directly modify interface `Dataset`, there could be a trait to separate the default methods and `Dataset extends Transactional, TxnTrait` (the `Transactional` for the javadoc mention rather than necessity).
```
public interface TxnTrait extends Transactional {
public default void exec(TxnType txnType, Runnable r) {
Txn.exec(this, txnType, r);
}
public default <X> X calc(TxnType txnType, Supplier<X> r) {
return Txn.calc(this, txnType, r);
}
...
}
```
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #872: JENA-2004 Implement functionality currently provided by Txn class directly in Transactional. Deprecate Txn class.
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #872:
URL: https://github.com/apache/jena/pull/872#issuecomment-734822362
The top-level question is whether this adds benefit to the user community. Our user community is diverse, some people are highly skilled developers, and some are highly skilled data experts who use programming as a means to an end (and may work in several languages).
`Txn` is public API and I don't think it helps to remove it.
Adding default methods that call `Txn.XYZ` would be good.
I also think that the traditional style begin/commit, ideally inside try-finally is what many users are familiar with, either because of JDBC experience or not really having adopted in their java the lambda functional style in their own code.
So my preference is add the default methods as calls to Txn, and have the Transactional javadoc be fairly neutral in opinion (I'll adds some review comments).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] rvesse commented on a change in pull request #872: JENA-2004 Implement functionality currently provided by Txn class directly in Transactional. Deprecate Txn class.
Posted by GitBox <gi...@apache.org>.
rvesse commented on a change in pull request #872:
URL: https://github.com/apache/jena/pull/872#discussion_r531487296
##########
File path: jena-arq/src/main/java/org/apache/jena/sparql/core/Transactional.java
##########
@@ -192,4 +174,110 @@ public default boolean promote() {
/** Say whether inside a transaction. */
public boolean isInTransaction() ;
+
+ /**
+ * Execute and return a value in a transaction with the given {@link TxnType transaction type}.
+ */
+ public default <T extends Transactional, X> X calc(TxnType txnType, Supplier<X> r) {
Review comment:
The downside of `default` methods is that since they become part of the interface implementations could choose to override them especially since you cannot declare them to be `final`
The advantage of the existing `Txn` helper class is that it provides a predictable and consistent behaviour regardless of the `Transactional` implementation it is operating over i.e. if you are using `Txn.executeRead()` it's always going to behave the same whereas calling `ds.executeRead()` could change behaviour depending on the dataset implementation.
##########
File path: jena-arq/src/main/java/org/apache/jena/sparql/core/Transactional.java
##########
@@ -31,32 +33,12 @@
* <pre> begin(WRITE) ... abort() or commit()</pre>
* <p>{@code end()} is optional but preferred.
* <p>
- * Helper code is available {@link Txn} so, for example:
- * <pre>Txn.executeRead(dataset, {@literal ()->} { ... sparql query ... });</pre>
- * <pre>Txn.executeWrite(dataset, {@literal ()->} { ... sparql update ... });</pre>
- * or use one of <tt>Txn.calculateRead</tt> and <tt>Txn.executeWrite</tt>
+ * In most cases instead of calling primitive methods such as {@code begin} and {@code commit }
+ * it makes more sense to use high-level default methods:
+ * <pre>dataset.executeRead({@literal ()->} { ... sparql query ... });</pre>
+ * <pre>dataset.executeWrite({@literal ()->} { ... sparql update ... });</pre>
+ * or use one of <tt>calculateRead</tt> and <tt>executeWrite</tt>
* to return a value for the transaction block.
- * <p>
Review comment:
Since the old style will still be supported and used by many users I don't think it makes sense to remove the old style from the Javadoc entirely
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] strangepleasures commented on pull request #872: JENA-2004 Implement functionality currently provided by Txn class directly in Transactional. Deprecate Txn class.
Posted by GitBox <gi...@apache.org>.
strangepleasures commented on pull request #872:
URL: https://github.com/apache/jena/pull/872#issuecomment-734991019
> Adding default methods that call `Txn.XYZ` would be good.
I'm not sure actually. Calling `Txn` from `Transactional` would introduce a circular dependency between them, which is not nice, especially because they belong to different packages.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on a change in pull request #872: JENA-2004 Implement functionality currently provided by Txn class directly in Transactional. Deprecate Txn class.
Posted by GitBox <gi...@apache.org>.
afs commented on a change in pull request #872:
URL: https://github.com/apache/jena/pull/872#discussion_r531587137
##########
File path: jena-arq/src/main/java/org/apache/jena/riot/resultset/rw/ReadAnything.java
##########
@@ -75,7 +74,7 @@ public static SPARQLResult read(String url, Context context) {
};
if ( ds.supportsTransactions() )
- return Txn.calculateWrite(ds, r);
+ return ds.calculateWrite(r);
Review comment:
If we go with keeping `Txn`, I prefer to not make these style changes.
##########
File path: jena-arq/src/main/java/org/apache/jena/sparql/core/Transactional.java
##########
@@ -31,32 +33,12 @@
* <pre> begin(WRITE) ... abort() or commit()</pre>
* <p>{@code end()} is optional but preferred.
* <p>
- * Helper code is available {@link Txn} so, for example:
- * <pre>Txn.executeRead(dataset, {@literal ()->} { ... sparql query ... });</pre>
- * <pre>Txn.executeWrite(dataset, {@literal ()->} { ... sparql update ... });</pre>
- * or use one of <tt>Txn.calculateRead</tt> and <tt>Txn.executeWrite</tt>
+ * In most cases instead of calling primitive methods such as {@code begin} and {@code commit }
Review comment:
"In most cases" begs the question of when is it appropriate to use begin-commit.
There will be application already existing that correctly uses begin-try-commit-finally and works just fine and we are keeping valid.
How about:
"Applications can call [examples e.g. dataset.executeRead] or use the lower-level primitives [dco for begin-commit].
By the way Txn will handle some nesting cases - no nested transactions but to cope with the case of an existing transaction.
##########
File path: jena-arq/src/main/java/org/apache/jena/system/Txn.java
##########
@@ -18,18 +18,22 @@
package org.apache.jena.system;
-import java.util.function.Supplier ;
-
import org.apache.jena.query.TxnType;
-import org.apache.jena.sparql.core.Transactional ;
+import org.apache.jena.sparql.core.Transactional;
+
+import java.util.function.Supplier;
-/** Application utilities for executing code in transactions.
+/**
+ * Application utilities for executing code in transactions.
+ * <p>
+ *
+ * @deprecated Use {@link Transactional } class directly instead
Review comment:
Not deprecated.
There is existing code out there and we've been encouraging Txn use so changing to deprecated like this feels rather sudden.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #872: JENA-2004 Implement functionality currently provided by Txn class directly in Transactional. Deprecate Txn class.
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #872:
URL: https://github.com/apache/jena/pull/872#issuecomment-800106577
See PR #954 which I hope is cautious step to get the Transactional interface extended so it is in Jena 4.0.0.
An alternative design with `TxnTrait` (the default methods added separately) might be better. This leaves `Transaction` cleaner as an implementation API.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #872: JENA-2004 Implement functionality currently provided by Txn class directly in Transactional. Deprecate Txn class.
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #872:
URL: https://github.com/apache/jena/pull/872#issuecomment-800114787
Continued on #954.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] strangepleasures commented on a change in pull request #872: JENA-2004 Implement functionality currently provided by Txn class directly in Transactional. Deprecate Txn class.
Posted by GitBox <gi...@apache.org>.
strangepleasures commented on a change in pull request #872:
URL: https://github.com/apache/jena/pull/872#discussion_r531628737
##########
File path: jena-arq/src/main/java/org/apache/jena/system/Txn.java
##########
@@ -18,18 +18,22 @@
package org.apache.jena.system;
-import java.util.function.Supplier ;
-
import org.apache.jena.query.TxnType;
-import org.apache.jena.sparql.core.Transactional ;
+import org.apache.jena.sparql.core.Transactional;
+
+import java.util.function.Supplier;
-/** Application utilities for executing code in transactions.
+/**
+ * Application utilities for executing code in transactions.
+ * <p>
+ *
+ * @deprecated Use {@link Transactional } class directly instead
Review comment:
@rvesse, @afs, Thanks for your feedback, I'll make the required changes.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org