Add rpc and local attribute #17

zklgame · 2024-01-04T15:43:58Z

This MR did:

add local attribute
add persistence in RPC

codecov · 2024-01-04T15:48:35Z

Codecov Report

Attention: 35 lines in your changes are missing coverage. Please review.

Comparison is base (409c5c2) 76.64% compared to head (1d7cb3d) 77.34%.

❗ Current head 1d7cb3d differs from pull request most recent head 72b3d64. Consider uploading reports for the commit 72b3d64 to get more accurate results

Files	Patch %	Lines
.../main/java/io/xcherry/core/rpc/RpcInterceptor.java	62.50%	3 Missing and 3 partials ⚠️
...rsistence/read_request/AppDatabaseReadRequest.java	61.53%	3 Missing and 2 partials ⚠️
...e/schema/local_attribute/LocalAttributeSchema.java	79.16%	3 Missing and 2 partials ⚠️
...rry/core/persistence/schema/PersistenceSchema.java	73.33%	2 Missing and 2 partials ⚠️
...va/io/xcherry/core/process/ProcessStartConfig.java	71.42%	2 Missing and 2 partials ⚠️
.../java/io/xcherry/core/state/AsyncStateOptions.java	57.14%	1 Missing and 2 partials ⚠️
...istence/LocalAttributeSchemaNotMatchException.java	0.00%	2 Missing ⚠️
.../java/io/xcherry/core/persistence/Persistence.java	94.73%	1 Missing and 1 partial ⚠️
...c/main/java/io/xcherry/core/utils/ProcessUtil.java	80.00%	0 Missing and 2 partials ⚠️
...ocal_attribute/LocalAttributeKeyValueSelector.java	80.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main      #17      +/-   ##
============================================
+ Coverage     76.64%   77.34%   +0.69%     
- Complexity      308      374      +66     
============================================
  Files            55       63       +8     
  Lines           959     1121     +162     
  Branches         58       74      +16     
============================================
+ Hits            735      867     +132     
- Misses          172      188      +16     
- Partials         52       66      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

longquanzheng · 2024-01-05T04:17:13Z

src/main/java/io/xcherry/core/exception/persistence/LocalAttributeNotFoundException.java

@@ -0,0 +1,8 @@
+package io.xcherry.core.exception.persistence;
+
+public class LocalAttributeNotFoundException extends RuntimeException {


Do we really need this exception? I don't remember a use case

If it's thrown when the attribute doesn't have a value, then I would prefer returning null. Because it's very common to start with empty/null and handling this exception is sometimes cumbersome

Sounds good. For local attributes, null should be enough. While for app database, we throw not found exception if a table or column does not exist.

longquanzheng · 2024-01-05T04:20:02Z

src/main/java/io/xcherry/core/exception/persistence/LocalAttributeSchemaNotMatchException.java

@@ -0,0 +1,8 @@
+package io.xcherry.core.exception.persistence;
+
+public class LocalAttributeSchemaNotMatchException extends RuntimeException {


Maybe LocalAttributeSchemaException extends ProcessDefinitionException

longquanzheng · 2024-01-05T04:21:53Z

src/main/java/io/xcherry/core/persistence/Persistence.java

+     * @param key       key of the local attribute.
+     * @param value     value of the local attribute.
+     */
+    public void upsertLocalAttribute(final String key, final Object value) {


probably just setLocalAttribute to pair with getLocalAttribute. You can keep the comment to make it more clear

longquanzheng · 2024-01-05T04:23:09Z

src/main/java/io/xcherry/core/persistence/read_request/LocalAttributeReadRequest.java

@@ -0,0 +1,66 @@
+package io.xcherry.core.persistence.read_request;


I feel like it's probably not a good practice to use underscore for package name in Java. I never see that in any popular libraries

True, making all lower case without _ is also recommended:
https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html

longquanzheng · 2024-01-05T04:24:44Z

src/main/java/io/xcherry/core/persistence/schema/local_attribute/LocalAttributeKeySchema.java

+
+@Getter
+@RequiredArgsConstructor(access = AccessLevel.PRIVATE)
+public class LocalAttributeKeySchema {


I feel like LocalAttributeDef is shorter and clear enough just like in iwf Java SDK

Here is to be consistent with existing app database names. We are using xxxSchema.

longquanzheng · 2024-01-05T04:30:11Z

src/main/java/io/xcherry/core/rpc/RPC.java

+     *
+     * @return the name as defined in a {@link RpcPersistenceReadRequest}.
+     */
+    String rpcPersistenceReadRequestName() default "";


Just a note that we need another PR to clean up things a little big to get rid of App database later and move back to a simpler model like iwf..

zklgame added 2 commits January 4, 2024 15:22

add local attributes

38bfbb7

add rpc persistence

1d7cb3d

zklgame requested a review from longquanzheng January 4, 2024 15:55

longquanzheng approved these changes Jan 5, 2024

View reviewed changes

refine

72b3d64

zklgame merged commit f5348f2 into main Jan 5, 2024
1 check passed

zklgame deleted the add_rpc_and_local_attribute branch January 5, 2024 07:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rpc and local attribute #17

Add rpc and local attribute #17

zklgame commented Jan 4, 2024 •

edited

Loading

codecov bot commented Jan 4, 2024 •

edited

Loading

longquanzheng Jan 5, 2024

longquanzheng Jan 5, 2024

zklgame Jan 5, 2024

longquanzheng Jan 5, 2024

longquanzheng Jan 5, 2024

longquanzheng Jan 5, 2024

zklgame Jan 5, 2024

longquanzheng Jan 5, 2024

zklgame Jan 5, 2024

longquanzheng Jan 5, 2024

		@@ -0,0 +1,8 @@
		package io.xcherry.core.exception.persistence;

		public class LocalAttributeNotFoundException extends RuntimeException {

		@@ -0,0 +1,8 @@
		package io.xcherry.core.exception.persistence;

		public class LocalAttributeSchemaNotMatchException extends RuntimeException {

		@@ -0,0 +1,66 @@
		package io.xcherry.core.persistence.read_request;

Add rpc and local attribute #17

Add rpc and local attribute #17

Conversation

zklgame commented Jan 4, 2024 • edited Loading

codecov bot commented Jan 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zklgame commented Jan 4, 2024 •

edited

Loading

codecov bot commented Jan 4, 2024 •

edited

Loading