bpsm / edn-java

a reader for extensible data notation
Eclipse Public License 1.0
100 stars 24 forks source link

Develop #41

Closed chrisbetz closed 9 years ago

chrisbetz commented 9 years ago

Hi,

I'd love to have Keyword and Symbol implement Serializable, so the resulting structure is serializable.

I also included a test to check stuff, but feel free to come back to me if you need further assistance in merging.

Cheers, Chris

chrisbetz commented 9 years ago

Sorry for that Java7 trouble. Hope you find this acceptable now :)

bpsm commented 9 years ago

I'm curious about why do you find that you need Symbol and Keyword to be java.io.Serializable. It would seem to me that they can already be serialized to EDN text. Why is that not sufficient in your case?

chrisbetz commented 9 years ago

Hi Ben,

thanks for asking. Background is, that I'm using edn-java to read config files (in EDN, obviously) to use them in my Apache Spark application.

As Spark is a distributed computing engine, it distributes stuff from one machine to another using Java serialization. Thus, I need to serialize my configuration.

Coming from a Clojure background, I'd like the keys to my config maps being keywords instead of Strings. However, as the config needs to be serializable, Keyword and Symbol (as Keyword depends on this) needs to be serializable.

Thus, with this small change, I could use edn-java and keywords, without this change I'd have to have either my own fork (with all the overhead) or I needed to go with Strings for map-keys.

So, I'd really, really appreciate if you could accept the pull request and release a new version, also, so I could get rid of my manually patched version ;)

Thanks again!

On Wed, Oct 14, 2015 at 9:19 PM, Ben Smith-Mannschott < notifications@github.com> wrote:

I'm curious about why do you find that you need Symbol and Keyword to be java.io.Serializable. It would seem to me that they can already be serialized to EDN text. Why is that not sufficient in your case?

— Reply to this email directly or view it on GitHub https://github.com/bpsm/edn-java/pull/41#issuecomment-148164760.

bpsm commented 9 years ago

Ah, I see. Though in the case of Keyword serialization is a little more complicated since Keywords are intended to be instance controlled such that kw1.equals(kw2) == (kw1 == kw2). That is, any given Keyword will exist only once.

    private Object writeReplace() {
        return new SerializationProxy(sym);
    }

    private void readObject(ObjectInputStream stream) {
        throw new InvalidObjectException("only proxy can be serialized");
    }

    private static class SerializationProxy implements Serializable {
        private static final long serialVersionUID = 1L;
        private final Symbol sym;
        private SerializationProxy(Symbol sym) {
            this.sym = sym;
        }
        private Object readResolve() throws ObjectStreamException {
            return newKeyword(sym);
        }
    }

Also, for consistency's sake Tag and TaggedValue would have to be made Serializable as well, though they are apparently not required by your concrete use case.

bpsm commented 9 years ago

Hi Chris, Please have a look at https://github.com/bpsm/edn-java/pull/42. Would that meet your needs?

chrisbetz commented 9 years ago

Hi,

That does look good, I'll try that on Spark on Monday and keep you posted. Thanks :)

Bye

Chris

Am 17.10.2015 um 18:50 schrieb Ben Smith-Mannschott notifications@github.com:

Hi Chris, Please have a look at #42. Would that meet your needs?

— Reply to this email directly or view it on GitHub.

chrisbetz commented 9 years ago

Hi Ben,

sorry, it took me some more days. I switched to the feature branch, tested it and everything works as a breeze.

So, looks great, thanks a lot!

I'll close this, as you track stuff in #42 .

Cheers, Chris

bpsm commented 9 years ago

https://github.com/bpsm/edn-java/pull/44