[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16365061#comment-16365061
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user stain commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
You may have better ideas on how to do something like 
[ParserConfigImpl](https://github.com/apache/commons-rdf/blob/fluent-parser/commons-rdf-api/src/main/java/org/apache/commons/rdf/api/io/ParserConfigImpl.java)
 so it is serializable.  For instance the 
[ParserSource](https://github.com/apache/commons-rdf/blob/fluent-parser/commons-rdf-api/src/main/java/org/apache/commons/rdf/api/io/ParserSource.java)
 beans might or might not be serializable depending on the implementation.

(Surely one that is just connected to an open InputStream is not 
serializable, but one that has just got an IRI should be serializable. I made 
the implementations package private: 
[IRIParserSource](https://github.com/apache/commons-rdf/blob/fluent-parser/commons-rdf-api/src/main/java/org/apache/commons/rdf/api/io/IRIParserSource.java)
 which meant I had to make a new 
[IRIImpl](https://github.com/apache/commons-rdf/blob/fluent-parser/commons-rdf-api/src/main/java/org/apache/commons/rdf/api/io/IRIImpl.java)
 to avoid Simple dependency)


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>Priority: Major
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16365054#comment-16365054
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user stain commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
Thanks, @ajs6f !

I found some old code where I had tried to make a fluent interface.. I 
managed to update it to the current master and sorted some issues.

I pushed it to the `fluent-parser` branch

See https://github.com/apache/commons-rdf/compare/fluent-parser 

I haven't implemented the `Parser` yet or written any tests.

Basically the idea is this:

```java
  Parsed p = rdf.parserBuilder()
  .syntax(RDFSyntax.JSONLD)
  .source("http://example.com/data.jsonld;)
  .parse();
```

or:

```java
rdf.parserBuilder()
  .syntax(RDFSyntax.TURTLE)
  .target(quad -> System.out.println(quad.getSubject()))  
  .source(Paths.get("/tmp/file.ttl").
  .async().parseAsync();
```

Now there is a set of interfaces, one for each step along the way, e.g. 
`NeedTarget`, and 
some internal `_` package interfaces to ensure consistency (but this can be 
flattened). Note that it is easier in this code to explore this in Eclipse with 
auto-complete as the interfaces have not been flattened yet.

It is implemented by a single `AbstractParserBuilder` which keeps all its 
state (except async executor) in a `ParserConfig` bean. The builder can be made 
immutable using `.build()` after which any change will make it mutable again.  
While it's mutable it will mutate the bean without any copies.

There is also a more low-level `Parser` which takes a `ParserConfig` - this 
is basically how the RDF implementations can be invoked.

I have not moved over the preflight checks in AbstractRDFParser there.

Feel free to use it as a starting ground or inspiration! It's quite hard to 
do fluent interfaces..


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>Priority: Major
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16364604#comment-16364604
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user ajs6f commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
Okay, I'll get to work on factoring out a (serializable) factory. Thanks 
for the discussion everyone! I think the parser types will be much stronger for 
it.


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>Priority: Major
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16364601#comment-16364601
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user asfgit closed the pull request at:

https://github.com/apache/commons-rdf/pull/43


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>Priority: Major
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16364535#comment-16364535
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user ajs6f commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
Hm... if @afs and @stain are both feeling reluctant to go this way... I 
would still be happy to merge it as-is and then work forward to the fluent-ish 
factory design (since @ansell has given a LGTM) and there is agreement that 
having fields not be `Optional`s is good in any case. @stain, would you like to 
see some checks against `null`-valued fields? I can certainly add those easily 
enough.


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>Priority: Major
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16361675#comment-16361675
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user stain commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
Picking up this - @ajs6f do you still think we should proceed along those 
lines? I am also reluctant to making the abstract factory (builder) 
serializable, but I can see the reasoning, particularly if you want to use this 
in Hadoop or something where you have a pre-made parser builder and then tell a 
different node to run it.

One thing I feel I need to check more is that there is no reading of the 
now-usually-null fields beyond the getters - I might rename them to make that 
clear.

I have put this PR into upstream branch COMMONSRDF-49 we can contribute to.


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>Priority: Major
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16258484#comment-16258484
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user ajs6f commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
@wikier My sense is that the right move here immediately is to merge this 
PR and then open a ticket to factor config (and a builder therefor) out of 
`AbstractRDFParser`. (A ticket I will be happy to work!)


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16254314#comment-16254314
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user ansell commented on a diff in the pull request:

https://github.com/apache/commons-rdf/pull/43#discussion_r151267617
  
--- Diff: 
commons-rdf-simple/src/main/java/org/apache/commons/rdf/simple/experimental/AbstractRDFParser.java
 ---
@@ -200,19 +200,19 @@
  * been set
  */
 public Optional getSourceIri() {
-return sourceIri;
+return Optional.ofNullable(sourceIri);
 }
 
-private Optional rdfTermFactory = Optional.empty();
-private Optional contentTypeSyntax = Optional.empty();
-private Optional contentType = Optional.empty();
-private Optional base = Optional.empty();
-private Optional sourceInputStream = Optional.empty();
-private Optional sourceFile = Optional.empty();
-private Optional sourceIri = Optional.empty();
+private RDF rdfTermFactory = null;
+private RDFSyntax contentTypeSyntax = null;
+private String contentType = null;
+private IRI base = null;
+private InputStream sourceInputStream = null;
--- End diff --

This may be a sticking point for actually enabling ``Serializable``, but 
the changes to not use optional field values look good to me.


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16254289#comment-16254289
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user ansell commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
Having ``Optional`` fields isn't impossible to serialise (as I said 
erroneously in the referenced comment), as you could always write custom 
serialise/deserialise code to support it, but it isn't supported by default, 
intentionally:

http://mail.openjdk.java.net/pipermail/jdk8-dev/2013-September/003274.html 

I haven't looked at the code/framework for a long time, but my recollection 
was that the parser was based on a single-threaded model after it came out of 
the multi-thread callable factory. If you are only expecting to run on a single 
thread, you may not have the support structures in place to run over multiple 
machines in a typical fashion, which is what ``Serializable`` has as one of its 
implied meanings. That would be a broader discussion though and shouldn't stop 
users from benefiting from serialisation in cases where they know it is 
safe/possible.

Overall, changing the instance variables to nullable raw types, and then 
using their accessors to see them as ``Optional`` is a compromise that allows 
serialisation without custom coding for it, and hence this PR looks good to me.


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16253697#comment-16253697
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user ajs6f commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
 


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16253687#comment-16253687
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user wikier commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
Having a little understanding on the concrete use case behind this PR, 
generally speaking I normally prefer having the factories/builders implementing 
`Serializable`. So in this concrete case I probably agree that the parser 
doesn't need that configuration. But again, I lack the background that 
motivated this request. Maybe having @ansell clarifying 
[`COMMONSRDF-49`](https://issues.apache.org/jira/browse/COMMONSRDF-49) may help 
to lead to a conclusion.

So, regarding the ongoing `0.5.0` release, I think it'd be better to keep 
this out of `RC2`, refine the requirements, and then reach an agreement that 
could be shipped in a future release.


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16253643#comment-16253643
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user ajs6f commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
I'd like to get @wikier an answer to his question. It sounds like we are 
_not_ comfortable merging this for `RC2` and he should go ahead without it, 
correct? 

If the consensus is that serializable config should be factored out of 
`AbstractRDFParser` (and I agree with that approach) than I think we should 
close `COMMONSRDF-49` and open a new ticket that is more exact (or edit 
`COMMONSRDF-49`) so that @ansell understands what to expect. I don't have the 
mojo in Commons RDF's Jira to do that.


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16253193#comment-16253193
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user stain commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
Agree with @afs that only the builder-factory bit should be serialized - 
obviously the actual parser which may be in progress of parsing is tricky to 
serialize. So we would need to perhaps clean up the class names/method to 
separate those conserns, without going too much towards a 
`AbstractBuilderFactorySingletonFactory`.


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16251700#comment-16251700
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user ajs6f commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
So the question is whether what is here now is a step forward.

I think it is, and I would want to do what is here whether or not we go 
onto a builder factoring. I intentionally did not add the marker interface to 
the type, and I think it's okay for the moment (especially as this is in the 
`experimental` package). I can and will do a builder refactoring, but I don't 
want to slow anyone down waiting for that.


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16250307#comment-16250307
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user afs commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
All I did was point out that it isn't logically serializable at all points 
in its usage. 

My suggestion is that there is design with a builder (serializable) and 
instance (not serializable) which separates these two concerns. YMMV.



> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16250172#comment-16250172
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user ajs6f commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
My impression is that @afs objects to the current design, but I haven't 
heard feedback on [my 
suggestion](https://github.com/apache/commons-rdf/pull/43#issuecomment-341721718).


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16250169#comment-16250169
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user wikier commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
Now that we have aborted `0.5.0-RC1`, do you think this should go in `RC2`?

Honestly I have no much background to really have an opinion on this PR.


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Stian Soiland-Reyes
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16237668#comment-16237668
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user ajs6f commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
Well, it's hard for me to say, because I'm trying to predict the 
architecture of a system I am only beginning to design.  :(

@afs, @sebbASF, how about I break out a separate type for serializable 
config and a builder for it and we take that forward? It might be enough and if 
it isn't we can revisit the question then. I could leave `AbstractRDFParser` 
as-is, although I'd rather alter it, both for the reasons @kinow gave in Jira 
and if I break out a config-builder to indicate the preferred usage. 


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Andy Seaborne
>Priority: Major
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16237558#comment-16237558
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user afs commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
AbstractRDFParser is stateful at some points in its lifecycle - input 
stream and the threadpool/futures.

Is this mixing a builder for the configuration, where serialization makes 
sense, with parser instantiation and binding to local resources, where 
serialization (of the clone) doesn't make sense?



> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Andy Seaborne
>Priority: Major
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[Bruno P. Kinoshita (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16237515#comment-16237515
 ] 

Bruno P. Kinoshita commented on COMMONSRDF-49:
--

>Relaying a question from sebb on the JIRA: 
>https://issues.apache.org/jira/browse/COMMONSRDF-49#comment-16236065 including 
>pointing out the long term restriction that comes with it.

There were other threads in the past discussing reducing/removing Serialization 
from components (e.g. Commons Math) as it increased the complexity for 
maintaining the component. Other components use Serialization to fulfil 
interface contracts (e.g. Commons Collections). I think the concern is valid, 
and unless we need to, we can avoid promising to maintain Serialization. But we 
can make it easier for a component to support serialization...

>From my POV, serializing a parser is useful for very long or very large ETL 
>when you may need to either pause and resume parsing (persisting the state of 
>the task in between) or move parsing tasks from one portion of the execution 
>environment to another. I'm not saying it's a no-brainer. :)

Indeed! And if you use classes in a context like Jenkins, or any other 
distributed environment, you may have to serialize objects over a stream.

>From what I understood, we are working around a limitation in Java8 Optional's 
>to be serialized. However, I don't see a serialVersionUID, and do't think the 
>parser implements Serializable right now. I checked out the pull request code 
>locally, looked at the AbstractRDFParser and RDFParser, and they don't seem to 
>implement Serializable.

So if I understand correctly, instead of having Optional as the 
instance type, after this pull request we will have SomeType as the instance 
field, methods will be using the Optional, and we will keep backward 
binary/API compatibility. The only difference being that users that want to 
serialize the parser, will be able to do so without having to think too hard 
how to bypass the Optional issue.

If that's correct, I think it's a good idea to make these changes in the class, 
but perhaps change the title of the this ticket? Perhaps "Avoid using Optional 
for instance fields" ? So that we can avoid any confusion.

> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Andy Seaborne
>Priority: Major
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16237455#comment-16237455
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user ajs6f commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
From my POV, serializing a parser is useful for very long or very large ETL 
when you may need to either pause and resume parsing (persisting the state of 
the task in between) or move parsing tasks from one portion of the execution 
environment to another. I'm not saying it's a no-brainer. :)

It may be better to have a subtype that is serializable but leave this type 
alone?


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Andy Seaborne
>Priority: Major
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16237400#comment-16237400
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

Github user afs commented on the issue:

https://github.com/apache/commons-rdf/pull/43
  
Relaying a question from sebb on the JIRA: 
https://issues.apache.org/jira/browse/COMMONSRDF-49#comment-16236065 including 
pointing out the long term restriction that comes with it.


> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Assignee: Andy Seaborne
>Priority: Major
>  Labels: parser
> Fix For: 0.6.0
>
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[Sebb (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16236065#comment-16236065
 ] 

Sebb commented on COMMONSRDF-49:


What is the use case for making the class serializable?

Whilst it's fairly simple to make a class serializable, in general it's very 
difficult to do it properly.
And adding Serializable places restrictions on future changes to the class.

> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Priority: Major
>  Labels: parser
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (COMMONSRDF-49) Make AbstractRDFParser serializable

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/COMMONSRDF-49?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16235981#comment-16235981
 ] 

ASF GitHub Bot commented on COMMONSRDF-49:
--

GitHub user ajs6f opened a pull request:

https://github.com/apache/commons-rdf/pull/43

COMMONSRDF-49: Make AbstractRDFParser serializable

Very simple approach-- I just exposed the values of the fields internally 
and made the accessors keep producing `Optional`.  My understanding is that 
any modern JVM will hotspot all the `isPresent` and similar calls into `null` 
checks anyway, so there should be no performance implications.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ajs6f/commons-rdf COMMONSRDF-49

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/commons-rdf/pull/43.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #43






> Make AbstractRDFParser serializable
> ---
>
> Key: COMMONSRDF-49
> URL: https://issues.apache.org/jira/browse/COMMONSRDF-49
> Project: Apache Commons RDF
>  Issue Type: New Feature
>  Components: simple
>Affects Versions: 0.3.0
>Reporter: Stian Soiland-Reyes
>Priority: Major
>  Labels: parser
>
> Raised by [~p_ansell] in [pull request 
> 25|(https://github.com/apache/incubator-commonsrdf/pull/25#discussion_r85436754]
> {quote}
> The use of optional here as a field type makes it impossible to serialise. 
> Need to have the raw values stored in fields if you want to support 
> serialisation in the future, which should otherwise be possible.
> {quote}
> The suggestion is to avoid {{Optional}} in the private fields of 
> {{AbstractRDFParser}} so it can be serialized - it can still be {{Optional}} 
> in the accessor methods.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)