Permalink
Browse files

cleanup, export all constants, better comments in JS, better logging

  • Loading branch information...
Dave Dopson
Dave Dopson committed Nov 18, 2011
1 parent cffa463 commit 4a11a9cfd8de3414a11332bd76bd8dede6eae1d8
Showing with 84 additions and 72 deletions.
  1. +70 −25 lib/zookeeper.js
  2. +14 −47 src/node-zk.cpp
View
@@ -15,14 +15,15 @@ var _ = require('underscore');
// Constructor
////////////////////////////////////////////////////////////////////////////////
-module.exports = ZooKeeper;
+exports = module.exports = ZooKeeper;
+module.exports.ZooKeeper = ZooKeeper; // for compatibility
function ZooKeeper() {
var self = this;
self._native = new NativeZk();
self._native.emit = function(ev, a1, a2, a3) {
- if(this.logger) this.logger("Emitting '" + ev + "' with args: " + a1 + ", " + a2 + ", " + a3);
+ if(self.logger) self.logger("Emitting '" + ev + "' with args: " + a1 + ", " + a2 + ", " + a3);
if(typeof a1 === 'ZooKeeper') {
- // callback returns the object. need to mangle this to return the wrapper instead
+ // the event is passing the native object. need to mangle this to return the wrapper instead
a1 = this;
}
self.emit(ev, a1, a2, a3);
@@ -31,32 +32,76 @@ function ZooKeeper() {
util.inherits(ZooKeeper, EventEmitter);
-exports = module.exports = ZooKeeper;
-module.exports.ZooKeeper = ZooKeeper; // for compatibility
-
////////////////////////////////////////////////////////////////////////////////
// Constants
////////////////////////////////////////////////////////////////////////////////
-// Other Stuff
-exports.ZOK = NativeZk.ZOK;
-exports.ZOO_EPHEMERAL = NativeZk.ZOO_EPHEMERAL;
-exports.ZOO_SEQUENCE = NativeZk.ZOO_SEQUENCE;
-
-// Permissions
-exports.ZOO_PERM_ADMIN = NativeZk.ZOO_PERM_ADMIN;
-exports.ZOO_PERM_ALL = NativeZk.ZOO_PERM_ALL;
-exports.ZOO_PERM_CREATE = NativeZk.ZOO_PERM_CREATE;
-exports.ZOO_PERM_DELETE = NativeZk.ZOO_PERM_DELETE;
-exports.ZOO_PERM_READ = NativeZk.ZOO_PERM_READ;
-exports.ZOO_PERM_WRITE = NativeZk.ZOO_PERM_WRITE;
-
-// Log Levels
-exports.ZOO_LOG_LEVEL_ERROR = NativeZk.ZOO_LOG_LEVEL_ERROR;
-exports.ZOO_LOG_LEVEL_WARN = NativeZk.ZOO_LOG_LEVEL_WARN;
-exports.ZOO_LOG_LEVEL_INFO = NativeZk.ZOO_LOG_LEVEL_INFO;
-exports.ZOO_LOG_LEVEL_DEBUG = NativeZk.ZOO_LOG_LEVEL_DEBUG;
-exports.ZOO_LOG_LEVEL_ERROR = NativeZk.ZOO_LOG_LEVEL_ERROR;
+// Events
+exports.on_closed = 'close';
+exports.on_connected = 'connect';
+exports.on_event_created = 'created';
+exports.on_event_deleted = 'deleted';
+exports.on_event_changed = 'changed';
+exports.on_event_child = 'child';
+exports.on_event_notwatching = 'notwatching';
+
+// Other Constants
+for(key in NativeZk) {
+ exports[key] = NativeZk[key];
+ // console.log(key + " = " + exports[key]);
+}
+/* Notable Constants:
+Permissions:
+ * ZOO_PERM_READ = 1
+ * ZOO_PERM_WRITE = 2
+ * ZOO_PERM_CREATE = 4
+ * ZOO_PERM_DELETE = 8
+ * ZOO_PERM_ADMIN = 16
+ * ZOO_PERM_ALL = 31
+
+Dunno:
+ * ZOO_EPHEMERAL = 1
+ * ZOO_SEQUENCE = 2
+
+States:
+ * ZOO_EXPIRED_SESSION_STATE = -112
+ * ZOO_AUTH_FAILED_STATE = -113
+ * ZOO_CONNECTING_STATE = 1
+ * ZOO_ASSOCIATING_STATE = 2
+ * ZOO_CONNECTED_STATE = 3
+
+Log Levels:
+ * ZOO_LOG_LEVEL_ERROR = 1
+ * ZOO_LOG_LEVEL_WARN = 2
+ * ZOO_LOG_LEVEL_INFO = 3
+ * ZOO_LOG_LEVEL_DEBUG = 4
+
+API Responses:
+ * ZOK = 0
+ * ZSYSTEMERROR = -1
+ * ZRUNTIMEINCONSISTENCY = -2
+ * ZDATAINCONSISTENCY = -3
+ * ZCONNECTIONLOSS = -4
+ * ZMARSHALLINGERROR = -5
+ * ZUNIMPLEMENTED = -6
+ * ZOPERATIONTIMEOUT = -7
+ * ZBADARGUMENTS = -8
+ * ZINVALIDSTATE = -9
+ * ZAPIERROR = -100
+ * ZNONODE = -101
+ * ZNOAUTH = -102
+ * ZBADVERSION = -103
+ * ZNOCHILDRENFOREPHEMERALS = -108
+ * ZNODEEXISTS = -110
+ * ZNOTEMPTY = -111
+ * ZSESSIONEXPIRED = -112
+ * ZINVALIDCALLBACK = -113
+ * ZINVALIDACL = -114
+ * ZAUTHFAILED = -115
+ * ZCLOSING = -116
+ * ZNOTHING = -117
+ * ZSESSIONMOVED = -118
+*/
////////////////////////////////////////////////////////////////////////////////
// Methods
View
@@ -25,49 +25,29 @@ namespace zk {
return; \
}
-static Persistent<String> on_connected, on_closed, on_event_created, on_event_deleted,
- on_event_changed, on_event_child, on_event_notwatching;
-
-static Persistent<Function> json_parse_func, json_stringify_func;
-static Persistent<String> HIDDEN_PROP_ZK, HIDDEN_PROP_HANDBACK;
static Persistent<String> data_as_buffer;
-#define DEFINE_SYMBOL(ev) ev = NODE_PSYMBOL(#ev)
-#define DEFINE_EVENT(t,ev) DEFINE_SYMBOL(ev); t->Set(ev, ev)
-
-#define NODE_SET_PROTOTYPE_METHOD_SIG(templ, name, argc, callback) \
-do { \
- Local<FunctionTemplate> argv [argc]; \
- for (int i = 0; i < argc; i ++) argv[i] = FunctionTemplate::New(); \
- v8::Local<v8::Signature> __callback##_SIG = v8::Signature::New(templ, argc, argv); \
- v8::Local<v8::FunctionTemplate> __callback##_TEM = \
- FunctionTemplate::New(callback, v8::Handle<v8::Value>(), \
- __callback##_SIG); \
- templ->PrototypeTemplate()->Set(v8::String::NewSymbol(name), \
- __callback##_TEM); \
-} while (0)
+#define DEFINE_STRING(ev,str) static Persistent<String> ev = NODE_PSYMBOL(str)
+DEFINE_STRING (on_closed, "close");
+DEFINE_STRING (on_connected, "connect");
+DEFINE_STRING (on_event_created, "created");
+DEFINE_STRING (on_event_deleted, "deleted");
+DEFINE_STRING (on_event_changed, "changed");
+DEFINE_STRING (on_event_child, "child");
+DEFINE_STRING (on_event_notwatching, "notwatching");
+
+#define DEFINE_SYMBOL(ev) DEFINE_STRING(ev, #ev)
+DEFINE_SYMBOL (HIDDEN_PROP_ZK);
+DEFINE_SYMBOL (HIDDEN_PROP_HANDBACK);
class ZooKeeper: public ObjectWrap {
public:
- static Persistent<FunctionTemplate> constructor_template;
-
static void Initialize(v8::Handle<v8::Object> target) {
HandleScope scope;
- Local<FunctionTemplate> t = FunctionTemplate::New(New);
- constructor_template = Persistent<FunctionTemplate>::New(t);
+ Local<FunctionTemplate> constructor_template = FunctionTemplate::New(New);
constructor_template->SetClassName(String::NewSymbol("ZooKeeper"));
constructor_template->InstanceTemplate()->SetInternalFieldCount(1);
- DEFINE_EVENT (constructor_template, on_closed);

This comment has been minimized.

Show comment Hide comment
@Woodya

Woodya Dec 16, 2011

Collaborator

is there an API change that made all of this code unnecessary?

@Woodya

Woodya Dec 16, 2011

Collaborator

is there an API change that made all of this code unnecessary?

This comment has been minimized.

Show comment Hide comment
@ddopson

ddopson Dec 16, 2011

Collaborator

The biggest API change was the removal of the native EventEmitter class. This required a rearchitecture of the plugin's structure to include a JS wrapper that adds the EventEmitter (~90% of the other native modules had to do the same thing). During that work, I also did some clean-up on the native code.

What you are seeing in these lines is my attempt to rework the macro definitions. I moved as much as I could to outside the class and tried to use a more simple pattern of MACRO_DEF, MACRO_USE, MACRO_USE, MACRO_USE. I found this made the code simpler to understand.

In my original pull request comments, I warned that this was a major rework and risked breaking changes; however, I did try to maintain backwards compatibility to the best of my ability. With only light test coverage, this was a challenge, and I have fixed several back-compat bugs that I introduced accidentally (**). However, to the best of my knowledge, any piece of code written to the original module should work with the latest version of this code. If this is not true, I'll be more than happy to fix any bugs you can find.

@ddopson

ddopson Dec 16, 2011

Collaborator

The biggest API change was the removal of the native EventEmitter class. This required a rearchitecture of the plugin's structure to include a JS wrapper that adds the EventEmitter (~90% of the other native modules had to do the same thing). During that work, I also did some clean-up on the native code.

What you are seeing in these lines is my attempt to rework the macro definitions. I moved as much as I could to outside the class and tried to use a more simple pattern of MACRO_DEF, MACRO_USE, MACRO_USE, MACRO_USE. I found this made the code simpler to understand.

In my original pull request comments, I warned that this was a major rework and risked breaking changes; however, I did try to maintain backwards compatibility to the best of my ability. With only light test coverage, this was a challenge, and I have fixed several back-compat bugs that I introduced accidentally (**). However, to the best of my knowledge, any piece of code written to the original module should work with the latest version of this code. If this is not true, I'll be more than happy to fix any bugs you can find.

- DEFINE_EVENT (constructor_template, on_connected);
- DEFINE_EVENT (constructor_template, on_event_created);
- DEFINE_EVENT (constructor_template, on_event_deleted);
- DEFINE_EVENT (constructor_template, on_event_changed);
- DEFINE_EVENT (constructor_template, on_event_child);
- DEFINE_EVENT (constructor_template, on_event_notwatching);
-
- DEFINE_SYMBOL (HIDDEN_PROP_ZK);
- DEFINE_SYMBOL (HIDDEN_PROP_HANDBACK);
NODE_SET_PROTOTYPE_METHOD(constructor_template, "init", Init);
NODE_SET_PROTOTYPE_METHOD(constructor_template, "close", Close);
@@ -179,17 +159,6 @@ class ZooKeeper: public ObjectWrap {
constructor_template->InstanceTemplate()->SetAccessor(String::NewSymbol("data_as_buffer"), DataAsBufferPropertyGetter, DataAsBufferPropertySetter);
target->Set(String::NewSymbol("ZooKeeper"), constructor_template->GetFunction());
-

This comment has been minimized.

Show comment Hide comment
@Woodya

Woodya Dec 16, 2011

Collaborator

json_* was just unused code?

@Woodya

Woodya Dec 16, 2011

Collaborator

json_* was just unused code?

This comment has been minimized.

Show comment Hide comment
@ddopson

ddopson Dec 16, 2011

Collaborator

Yes, it was unused. It looks like the original intent was to be able to call the JSON.parse method from C, but that was never done. Likely, any feature intended to be implemented would be better served by living in the JS layer.

@ddopson

ddopson Dec 16, 2011

Collaborator

Yes, it was unused. It looks like the original intent was to be able to call the JSON.parse method from C, but that was never done. Likely, any feature intended to be implemented would be better served by living in the JS layer.

- // prepare handles for JSON parse and stringify functions
- Local<Value> json_ = Context::GetCurrent()->Global()->Get(String::NewSymbol("JSON"));
- assert (json_->IsObject());
- Local<Value> json_parse = json_->ToObject()->Get(String::NewSymbol("parse"));
- assert (json_parse->IsFunction());
- json_parse_func = Persistent<Function>::New (Local<Function>::Cast(json_parse));
-
- Local<Value> json_stringify = json_->ToObject()->Get(String::NewSymbol("stringify"));
- assert (json_stringify->IsFunction());
- json_stringify_func = Persistent<Function>::New (Local<Function>::Cast(json_stringify));
}
static Handle<Value> New (const Arguments& args) {
@@ -267,7 +236,6 @@ class ZooKeeper: public ObjectWrap {
LOG_ERROR (("zookeeper_init returned 0!"));
return false;
}
- //handle_.ClearWeak();
Ref();
ev_init (&zk_io, &zk_io_cb);
ev_init (&zk_timer, &zk_timer_cb);
@@ -723,8 +691,7 @@ class ZooKeeper: public ObjectWrap {
bool data_as_buffer;
};
-Persistent<FunctionTemplate> ZooKeeper::constructor_template;
-}
+} // namespace "zk"
extern "C" void init(Handle<Object> target) {
zk::ZooKeeper::Initialize(target);

0 comments on commit 4a11a9c

Please sign in to comment.