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

CURATOR-728: Not issue ZooKeeper::create if possible in ZkPaths::mkdirs #518

Merged

Conversation

kezhuw
Copy link
Member

@kezhuw kezhuw commented Jan 3, 2025

ZkPaths::mkdir("/bar/foo") will not run into NoAuthException if
"/bar/foo" exists, and we have READ permission to "/bar/foo" but not
CREATE permission to "/bar".

@@ -282,28 +282,29 @@ public static void mkdirs(ZooKeeper zookeeper, String path, boolean makeLastNode
*/
public static void mkdirs(
ZooKeeper zookeeper,
String path,
final String path,
Copy link
Member

Choose a reason for hiding this comment

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

Params are by default not be mutated. I suppose we don't need final to params.

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

@kezhuw
Copy link
Member Author

kezhuw commented Jan 7, 2025

Hmm, I pushed to apache/curator. I will delete this pr branch once merged. Sorry for the mistake.

@kezhuw kezhuw force-pushed the CURATOR-728-ZkPaths-mkdirs-not-issue-create-if-possible branch 2 times, most recently from ed2d3ae to 0fb6f3a Compare January 7, 2025 06:42
Copy link
Member Author

@kezhuw kezhuw left a comment

Choose a reason for hiding this comment

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

cc @tisonkun @pfcoperez

I made some changes to test since last review. I am going to merge this if no doubt.

@@ -224,7 +224,7 @@
<dependency>org.apache.curator:curator-recipes</dependency>
<dependency>org.apache.curator:curator-client</dependency>
</dependenciesToScan>
<excludedGroups>master</excludedGroups>
<excludedGroups>master,ZOOKEEPER-2590</excludedGroups>
Copy link
Member Author

Choose a reason for hiding this comment

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

ZOOKEEPER-2590 is never landed in 3.7 though it is tagged with 3.7.3.

@@ -31,7 +31,7 @@
<artifactId>curator-test-zk38</artifactId>

<properties>
<zookeeper-38-version>3.8.3</zookeeper-38-version>
<zookeeper-38-version>3.8.4</zookeeper-38-version>
Copy link
Member Author

Choose a reason for hiding this comment

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

ZOOKEEPER-2590 is landed in 3.8.4.

fail("Expected NoAuthException when not authorized to read new node grandparent");
} catch (KeeperException.NoAuthException noAuthException) {
assertEquals(noAuthException.getPath(), "/bat");
Copy link
Member Author

Choose a reason for hiding this comment

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

The old test reported NoAuth on creating /bat/bost. I think this is not the target CURATOR-715 trying to assert.

`ZkPaths::mkdir("/bar/foo")` will not run into `NoAuthException` if
"/bar/foo" exists, and we have `READ` permission to "/bar/foo" but not
`CREATE` permission to "/bar".
@kezhuw kezhuw force-pushed the CURATOR-728-ZkPaths-mkdirs-not-issue-create-if-possible branch from 0fb6f3a to ba387f2 Compare January 8, 2025 02:17
Copy link
Contributor

@pfcoperez pfcoperez left a comment

Choose a reason for hiding this comment

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

Thanks for this change @kezhuw . This makes the mkdirs ACLs constraints less restrictive which is a great improvement 🙏

@kezhuw kezhuw merged commit 476268a into master Jan 9, 2025
10 checks passed
@kezhuw kezhuw deleted the CURATOR-728-ZkPaths-mkdirs-not-issue-create-if-possible branch January 9, 2025 02:18
@yang-wei
Copy link

@kezhuw thank you for fixing this. It helps to unblock our upgrade to Zookeeper 3.9.3. Do you know when will be the next release that include this patch ?

yang-wei pushed a commit to yang-wei/curator that referenced this pull request Jan 14, 2025
…rs (apache#518)

`ZkPaths::mkdir("/bar/foo")` will not run into `NoAuthException` if
"/bar/foo" exists, and we have `READ` permission to "/bar/foo" but not
`CREATE` permission to "/bar".
@kezhuw
Copy link
Member Author

kezhuw commented Jan 15, 2025

@yang-wei There is no schedule for next release yet. You could post mail to dev mailing list to let the community know.

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.

4 participants