nathanmarz / cascalog

Data processing on Hadoop without the hassle.
Other
1.38k stars 178 forks source link

Issue #261 - added a couple functions for JCascalog to call into since #262

Closed dkincaid closed 9 years ago

dkincaid commented 9 years ago

it can't deal with ClojureFlow records. Added a test to check Api.compileFlow() method.

Also added synchronized keyword around all of the public static methods in Api.

Added Api.flowDef() method so that JCascalog can get a hold of the FlowDef object if desired.

Quantisan commented 9 years ago

@dkincaid is there downside to making all those methods synchronized?

kyrill007 commented 9 years ago

There is really no downside to making these methods synchronized. The problem is that this Api class is completely stateful (inside wrapped clojure calls) and not-thread safe. We really need to put a Javadoc comment on this class explaining to users that any aggregate/compound operations on the Api class should be made within the context of Api.class lock (i.e. synchornized(Api.class) {...}). Making the methods synchronized only partially resolves the thread safety problem, users need to understand that Api class calls are often compound and it is precisely the compound coarse-grained invocations that should be guarded with a lock.

dkincaid commented 9 years ago

Could we get this merged into the code base before the next release?

Quantisan commented 9 years ago

@dkincaid @kyrill007 stating the nuance of the class in the javadoc sounds good to me. do you guys want to add a few comments into the docstring please?

kyrill007 commented 9 years ago

I'll write the Javadocs for this class tomorrow.

kyrill007 commented 9 years ago

Comments added.

Quantisan commented 9 years ago

thanks gentlemen!