Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for container and TTL nodes #255

Closed
wants to merge 12 commits into from

Conversation

dreusel
Copy link
Contributor

@dreusel dreusel commented Dec 11, 2020

Description

Enables creating container nodes and TTL nodes

Motivation and Context

Fixes #210

How Has This Been Tested?

For both cases, it can take anywhere between 20 seconds and way more before Zookeeper actually cleans up the nodes

For container nodes

const Zookeeper = require('zookeeper');

const zkClient = new Zookeeper({
	connect: 'localhost:2181',
	timeout: 5000,
	debug_level: Zookeeper.ZOO_LOG_LEVEL_WARN,
	host_order_deterministic: false,
});

zkClient.on('connect', async () => {
	const nodeName = `/containerTest-${('' + Math.random()).split('.')[1]}`;
	await zkClient.create(`${nodeName}`, null, Zookeeper.ZOO_CONTAINER).catch((e) => console.log('create failed', e));
	await zkClient.create(`${nodeName}/child`).catch(console.log);
	await zkClient.delete_(`${nodeName}/child`).catch(console.log);
	const start = Date.now();
	while (true) {
		await new Promise(r => setTimeout(r, 1000));
		try {
			await zkClient.exists(nodeName);
			console.log(`it still exists after ${Date.now() - start}`);
		} catch(e) {
			console.log(`it was deleted after ${Date.now() - start}`, e);
			break;
		};
	}
	zkClient.close();
});

zkClient.init();

For TTL nodes:

const Zookeeper = require('zookeeper');

const zkClient = new Zookeeper({
	connect: 'localhost:2181',
	timeout: 5000,
	debug_level: Zookeeper.ZOO_LOG_LEVEL_WARN,
	host_order_deterministic: false,
});

zkClient.on('connect', async () => {
	const nodeName = `/ttlTest-${('' + Math.random()).split('.')[1]}`;
	console.log('nodeName: ' + nodeName);
	await zkClient.create(`${nodeName}`, '', Zookeeper.ZOO_PERSISTENT_WITH_TTL, 1000).catch((e) => console.log('create failed', e));
	const start = Date.now();
	while (true) {
		await new Promise(r => setTimeout(r, 1000));
		try {
			await zkClient.exists(nodeName);
			console.log(`it still exists after ${Date.now() - start}`);
		} catch(e) {
			console.log(`it was deleted after ${Date.now() - start}`, e);
			break;
		};
	}
	zkClient.close();
});

zkClient.init();

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

@DavidVujic DavidVujic self-requested a review December 11, 2020 20:44
@dreusel dreusel changed the title Support for container nodes Support for container and TTL nodes Dec 11, 2020
@DavidVujic
Copy link
Collaborator

Very cool!

I'll have a look within a couple of days, not more than a week.

@DavidVujic
Copy link
Collaborator

One thing that I just thought of is Windows support. Currently there is a (undocumented) workaround for Windows clients: because of not yet solved issues in the official C client, the version built on windows is 3.4.14 (some background can be found in the changelog).

What I'm thinking about is that it maybe is necessary to handle that, by some "if windows" clause, or if it is possible to determine the current client version running, and branch out with if/else?

@dreusel
Copy link
Contributor Author

dreusel commented Dec 11, 2020

AH, yeah. Can you tell me what #ifdef to use? Then I'll put that in. Unfortunately I don't have Windows available to test on though.
I suppose we'd #else define the constants as -1 (or something) then, so we can throw if people try to use them in a Windows environment.

@dreusel
Copy link
Contributor Author

dreusel commented Dec 11, 2020

Does the exception also apply to cygwin?

@DavidVujic
Copy link
Collaborator

DavidVujic commented Dec 17, 2020

Hello again @dreusel!

I'm sorry for the delay. I think I need more time to test, review and give feedback. Hope that it is ok!

@dreusel
Copy link
Contributor Author

dreusel commented Dec 18, 2020

That's okay 👍

@DavidVujic
Copy link
Collaborator

Hi @dreusel!

Your pull request inspired me to try and fix the Windows issue with using an outdated ZooKeeper Client version. And I think I've succeeded 😄

Now, all platforms will use v.3.5.8 of the C client code and there is no longer any need to handle Windows separately for the Container and TTL features.

This means that your Pull Request need some updates.

But don't worry about the prebuilds, I think it is totally fine to just delete them. I will build new ones in a separate release build.

I will also have a look at the changes in your PR and share some feedback.

If you are on vacay - no worries! Come back to this PR when you have an opportunity, there is no hurry from my point of view.

lib/zookeeper.js Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
lib/typedeclarations.d.ts Outdated Show resolved Hide resolved
@DavidVujic
Copy link
Collaborator

DavidVujic commented Jan 24, 2021

Hi again @dreusel!

Do you want to continue with this PR?

@dreusel
Copy link
Contributor Author

dreusel commented Jan 27, 2021

Hey there @DavidVujic , sorry, I was planning to continue on it but I didn't find the time.

@dreusel
Copy link
Contributor Author

dreusel commented Jan 27, 2021

I've also removed the win32 workarounds, but I dont have time to actually test it.

@DavidVujic
Copy link
Collaborator

Great @dreusel, thank you!

I'm planning to review and test this within a couple of days and want to release a new version next week. I will also will have a closer look and take your good suggestions about the API in consideration. Thank you for doing this!

@DavidVujic
Copy link
Collaborator

@dreusel I'm going to create a new feature branch and add your changes to it, together with updates to the prebuilds, readme and the feedback in this PR.

I'll squash and rebase the commits before merging, and will make sure your name will be in the commits (both our names will be there as "co-authored by").

I will reference the new PR to this one too, so we can keep track of the history and conversations.

@@ -126,6 +126,7 @@ Have a look at the code in the [examples](./examples) folder: with __master__, _
* `connect(options, connect_cb)`
* `close()`
* `a_create(path, data, flags, path_cb)`
* `a_createTtl(path, data, flags, path_cb)`
* `mkdirp(path, callback(Error))`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(will fix in separate branch): missing ttl parameter.

@dreusel
Copy link
Contributor Author

dreusel commented Jan 31, 2021

Sounds good @DavidVujic! Let me know if there's anything specific you want my help with

@DavidVujic
Copy link
Collaborator

Closing this one. Thank you for adding this feature! I think this package is the only one out there with TTL and Container support for Node.js. Great work @dreusel!

Merged with co-authored: b625f40

@DavidVujic DavidVujic closed this Jan 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ZooKeeper 3.5.5: new node types
2 participants