[WIP]Add tryLock REST API for distributed lock by mgao0 · Pull Request #1509 · apache/helix

mgao0 · 2020-11-03T22:52:08Z

Issues

My PR addresses the following Helix issues and references them in the PR description:

(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)

Description

Here are some details about my PR, including screenshots of any UI changes:

(Write a concise description including what, why, how)

Tests

The following tests are written for this issue:

(List the names of added unit/integration tests)

The following is the result of the "mvn test" command on the appropriate module:

(Before CI test pass, please copy & paste the result of "mvn test")

Documentation (Optional)

In case of new functionality, my PR adds documentation in the following wiki page:

(Link the GitHub wiki you added)

Commits

My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Code Quality

My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)

junkaixue · 2020-11-05T00:24:00Z

+      String lockPath = null;
+      if (_lockScope != null) {
+        lockPath = _lockScope.getPath();
+      }
+
+      if (_lockPath != null) {
+        lockPath = _lockPath;
+      }


I can understand the logic. But it seemed not be a good readable code.

If we think lockPath is necessary:

if (_lockScope != null) {
_lockPath = _lockScope.getPath();
}

junkaixue · 2020-11-05T00:25:03Z

+  private static final String ZK_ADDR = "zkAddress";
+  private static final String LOCK_PATH = "lockPath";
+  private static final String LOCK_TIMEOUT = "lockTimeout";
+  private static final String USER_ID = "userId";
+  private static final String LOCK_MSG = "lockMsg";
+  private static final String LOCK_SCOPE_PROPERTY = "property";
+  private static final String PATH_KEYS = "pathKeys";
+  private static final String PATH_KEY_DELIMITER = "\\|";


Let's create a constant class to hold them.

junkaixue · 2020-11-05T00:26:22Z

+  @ResponseMetered(name = HttpConstants.WRITE_REQUEST)
+  @Timed(name = HttpConstants.WRITE_REQUEST)
+  @POST
+  @Path("lock")


Do you mean the URI "xxxx/locks/lock" ? Shall we call it "acquireLock"?

Do you mean the URI "xxxx/locks/lock" ? Shall we call it "acquireLock"?
Write

In the REST API design, we don't expect a verb to represent a resource in the endpoint: the method POST/PUT/DELETE already tells the purpose.

@mgao0 Is this request "idempotent"? If one calls this endpoint N times with the same payload, will there be N locks, or just 1 lock?

junkaixue · 2020-11-05T00:26:41Z

+  @POST
+  @Path("lock")
+  public Response tryLock(String content) {
+    System.out.println("Payload:\n" + content);


Change this to log.

junkaixue · 2020-11-05T00:28:29Z

+          "Lock path is not found or lock scope could not be generated, please examine the request.");
+    }
+
+    return result ? OK(JSONRepresentation(lock)) : serverError(String.format(


You will not see the serverError part, right? Because it already returns badrequst in previous catch.

junkaixue · 2020-11-05T00:29:51Z

+    LockInfo lockInfo;
+    try {
+      // Parse payload data
+      ZNRecord lockContent = toZNRecord(content);


It's not a good idea to let input content to be ZNRecord. User has to input "simpleFields, listFields, mapFields" even they dont use it. I would suggest to use plain json format.

junkaixue · 2020-11-05T00:34:31Z

    private String _userId;
    private long _timeout;
    private String _lockMsg;
+    private String _lockPath;


What if lockPath is confilct the lockPath in lockScope.

zhangmeng916 · 2020-11-05T01:05:45Z

-          ZooKeeperAccessor.class.getPackage().getName(),
-          MetadataStoreDirectoryAccessor.class.getPackage().getName()
-      }),
+      new String[]{AbstractHelixResource.class.getPackage().getName(), NamespacesAccessor.class


The format is a bit weird. Can you revert them to previous one?

zhangmeng916 · 2020-11-05T01:29:19Z

+    // Perform try lock
+    boolean result;
+    try {
+      result = lock.tryLock();


This reminds me a question from our users. They wondered whether they can distinguish between zk error and lock owned by others. From our current implementation of zkclient, I think the answer is no, they are both returned as zero. could you double check that part? If that's the case, you may want to comment somewhere.

zhangmeng916 · 2020-11-05T01:30:24Z

+    }
+  }
+
+  @ResponseMetered(name = HttpConstants.WRITE_REQUEST)


To be consistent with Java API, this one should be tryLock.
Actually I suggest to think over all APIs before finalizing a specific one. E.g. for unlocking a lock or getting lock information, etc. I feel unlock should be similar to this, as an action, instead of DELETE operation. For get lock information, what information will you provide to users?

zhangmeng916 · 2020-11-05T01:32:31Z

+    if (znRecord == null) {
+      return null;
+    }
+    // Parse user iod


zhangmeng916 · 2020-11-05T01:33:46Z

+      throw new Exception("Please check the timeout value input, could not convert to long.");
+    }
+
+    // Parse lock message


There are a couple comments like this, which I think the code is self explanatory. You may just remove them for concise code.

zhangmeng916 · 2020-11-05T01:36:32Z

+          pathKeys.add(participantName);
+        }
+      }
+      default:


Your previous try catch already excluded all other cases, so you don't need default here.

zhangmeng916 · 2020-11-05T01:46:57Z

+    String CLUSTER = "cluster";
+    String RESOURCE = "resource";
+    String PARTICIPANT = "participant";


These should be defined as final static in the beginning too.

zhangmeng916 · 2020-11-05T01:47:54Z

+  private static final String LOCK_MSG = "lockMsg";
+  private static final String LOCK_SCOPE_PROPERTY = "property";
+  private static final String PATH_KEYS = "pathKeys";
+  private static final String PATH_KEY_DELIMITER = "\\|";


This definition doesn't look right. btw, it is not used anywhere.

zhangmeng916 · 2020-11-05T01:49:31Z

+  }
+
+  @Test(dependsOnMethods = "testLockAccessorWithHelixLockScope")
+  public void testLockAccessorWithUnknownScope() throws JsonProcessingException {


nit: maybe rename it to general scope etc.

narendly · 2020-11-15T23:11:32Z

        <artifact name="snakeyaml" m:classifier="sources" ext="jar"/>
    </dependency>
 		<dependency org="org.apache.helix" name="helix-core" rev="1.0.2-SNAPSHOT" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+				<dependency org="org.apache.helix" name="helix-lock" rev="1.0.2-SNAPSHOT" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>


Nit: fix indentation

narendly · 2020-11-15T23:13:40Z

+  @Path("lock")
+  public Response tryLock(String content) {
+    System.out.println("Payload:\n" + content);
+    ZKDistributedNonblockingLock lock;


Nit: probably better if we declare it as an interface?

narendly · 2020-11-15T23:14:54Z

+      // Build a lock object
+      lock = new ZKDistributedNonblockingLock.Builder().setLockMsg(lockInfo.getMessage())
+          .setUserId(lockInfo.getOwner()).setTimeout(lockInfo.getTimeout())
+          .setZkAddress(lockLocator.getZkAddress()).setLockPath(lockLocator.getLockPath()).build();


Let's be careful here - what would the zk address be set as if the user was using it on multi-zk mode?

narendly · 2020-11-15T23:15:23Z

+          .setUserId(lockInfo.getOwner()).setTimeout(lockInfo.getTimeout())
+          .setZkAddress(lockLocator.getZkAddress()).setLockPath(lockLocator.getLockPath()).build();
+    } catch (Exception e) {
+      return badRequest(e.getMessage());


Should we log this as well?

narendly · 2020-11-15T23:16:27Z

+      return badRequest(
+          "Lock path is not found or lock scope could not be generated, please examine the request.");


We need to let the caller know what the request was. Consider embedding that information in the exception message.

Should we add a server-side exception log here as well?

narendly · 2020-11-15T23:18:50Z

+    if (znRecord == null) {
+      return null;
+    }


I'm not sure if this is a good pattern to use. Usually these functions expect a non-null values as parameters. Should you do a null check on the zn record before calling parseLockLocator()? This is a debatable point, but maybe worth giving some consideration.

narendly · 2020-11-15T23:19:49Z

+    if (znRecord == null) {
+      return null;
+    }


same - should we do a null check on znrecord in the beginning?

narendly · 2020-11-15T23:20:25Z

+    try {
+      lockTimeout = Long.parseLong(znRecord.getSimpleField(LOCK_TIMEOUT));
+    } catch (NumberFormatException e) {
+      throw new Exception("Please check the timeout value input, could not convert to long.");


let's let the caller know what the value is?

narendly · 2020-11-15T23:22:09Z

+    String CLUSTER = "cluster";
+    String RESOURCE = "resource";
+    String PARTICIPANT = "participant";


It looks like we already have the enum for it. why can't we just use the enum instead of defining new static constants?

Either add a method in the enum that returns its lower-case equivalent or just do a enum.getString().toLowerCase().

narendly · 2020-11-15T23:24:24Z

+    try {
+      property = HelixLockScope.LockScopeProperty.valueOf(lockScopeProperty);
+    } catch (IllegalArgumentException e) {
+      throw new Exception(String.format(
+          "User requested lock scope property %s, but this property does not exist in HelixLockScope.",
+          lockScopeProperty));
+    }


I think there's a shorter way of doing this. Try googling? It looks like Enums.getIfPresent(Blah.class, "A")...

narendly · 2020-11-15T23:25:14Z

+        }
+      }
+      default:
+        // Not gonna happen


This doesn't look like a good example of a JavaDoc comment. Let's try to make it more descriptive or remove it altogether.

narendly · 2020-11-15T23:25:36Z

+    }
+    if (pathKeys.isEmpty()) {
+      throw new Exception(
+          "Not enough path keys are provided to construct the lock path. Please check the request.");


Let the user know what the request was. Also do we need to log it on the server side?

jiajunwang · 2021-05-07T18:10:14Z

Close due to inactive.

Add tryLock REST API for distributed lock

c7603cf

junkaixue reviewed Nov 5, 2020

View reviewed changes

zhangmeng916 reviewed Nov 5, 2020

View reviewed changes

narendly reviewed Nov 15, 2020

View reviewed changes

jiajunwang closed this May 7, 2021

		return badRequest(
		"Lock path is not found or lock scope could not be generated, please examine the request.");

Conversation

mgao0 commented Nov 3, 2020

Issues

Description

Tests

Documentation (Optional)

Commits

Code Quality

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

narendly Nov 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

narendly Nov 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiajunwang commented May 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

narendly Nov 15, 2020 •

edited

Loading

narendly Nov 15, 2020 •

edited

Loading