You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hugegraph.apache.org by GitBox <gi...@apache.org> on 2023/01/19 10:13:51 UTC
[GitHub] [incubator-hugegraph] imbajin opened a new pull request, #2095: refact: mark TODO in the license problem code & use standard UTF-8
imbajin opened a new pull request, #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095
also fix some tiny problem
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#issuecomment-1412409523
TODO: move RAT(check) from build action --> `check-license` action
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1098858614
##########
hugegraph-test/src/main/java/org/apache/hugegraph/tinkerpop/StructureBasicSuite.java:
##########
@@ -54,9 +54,8 @@
import org.junit.runners.model.RunnerBuilder;
import org.junit.runners.model.Statement;
-import org.apache.hugegraph.dist.RegisterUtil;
-
/**
+ * TODO: lack license header & LICENSE reference
Review Comment:
and this @zyxxoo
##########
hugegraph-test/src/main/java/org/apache/hugegraph/tinkerpop/ProcessBasicSuite.java:
##########
@@ -98,6 +98,7 @@
import org.apache.hugegraph.dist.RegisterUtil;
/**
+ * TODO: lack license header & LICENSE reference
Review Comment:
@zyxxoo and also handle this, remove it if we fixed it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1098855517
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -17,19 +17,22 @@
package org.apache.hugegraph.util;
-import java.io.UnsupportedEncodingException;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import java.io.IOException;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.Base64;
import java.util.UUID;
-import org.mindrot.jbcrypt.BCrypt;
-
import org.apache.hugegraph.HugeException;
import org.apache.hugegraph.backend.serializer.BytesBuffer;
+import org.mindrot.jbcrypt.BCrypt;
+
import com.google.common.base.CharMatcher;
/**
+ * TODO: lack license header & LICENSE reference
Review Comment:
> [JanusGraph/janusgraph@`master`/janusgraph-core/src/main/java/org/janusgraph/util/encoding/StringEncoding.java](https://github.com/JanusGraph/janusgraph/blob/master/janusgraph-core/src/main/java/org/janusgraph/util/encoding/StringEncoding.java?rgh-link-date=2023-01-20T12%3A37%3A02Z) The janusgraph also is APV2 license, should i keep the janusgraph origin license? should i modify our license file?
same as the [previous one ](https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1098853204)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1101438531
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -92,33 +96,21 @@ public static int getAsciiByteLength(String value) {
}
public static byte[] encode(String value) {
- try {
- return value.getBytes("UTF-8");
- } catch (UnsupportedEncodingException e) {
- throw new HugeException("Failed to encode string", e);
- }
+ return value.getBytes(UTF_8);
Review Comment:
> please don't change the logic
it's recommended by the lint, and here is the differ:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] javeme commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1101532429
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -14,19 +14,22 @@
package org.apache.hugegraph.util;
-import java.io.UnsupportedEncodingException;
+import static java.nio.charset.StandardCharsets.UTF_8;
Review Comment:
don't import static as possible, import StandardCharsets instead, and can define a static final UTF8 = StandardCharsets.UTF_8
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -137,18 +129,23 @@ public static byte[] compress(String value) {
}
public static byte[] compress(String value, float bufferRatio) {
- BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE,
- bufferRatio);
- return buf.bytes();
+ try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
+ return buf.bytes();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
}
public static String decompress(byte[] value) {
return decompress(value, LZ4Util.DEFAULT_BUFFER_RATIO);
}
public static String decompress(byte[] value, float bufferRatio) {
- BytesBuffer buf = LZ4Util.decompress(value, BLOCK_SIZE, bufferRatio);
- return decode(buf.array(), 0, buf.position());
+ try (BytesBuffer buf = LZ4Util.decompress(value, BLOCK_SIZE, bufferRatio)) {
+ return decode(buf.array(), 0, buf.position());
+ } catch (IOException e) {
Review Comment:
seems don't need to catch IOException
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -92,33 +96,21 @@ public static int getAsciiByteLength(String value) {
}
public static byte[] encode(String value) {
- try {
- return value.getBytes("UTF-8");
- } catch (UnsupportedEncodingException e) {
- throw new HugeException("Failed to encode string", e);
- }
+ return value.getBytes(UTF_8);
Review Comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] codecov[bot] commented on pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#issuecomment-1396746985
# [Codecov](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2095](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3694974) into [master](https://codecov.io/gh/apache/incubator-hugegraph/commit/fc6e3702da5373c9f9d994e7233c14a7d74d610c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fc6e370) will **decrease** coverage by `17.47%`.
> The diff coverage is `55.55%`.
```diff
@@ Coverage Diff @@
## master #2095 +/- ##
=============================================
- Coverage 65.94% 48.47% -17.47%
+ Complexity 977 652 -325
=============================================
Files 479 479
Lines 39655 39590 -65
Branches 5578 5554 -24
=============================================
- Hits 26150 19193 -6957
- Misses 10935 18329 +7394
+ Partials 2570 2068 -502
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../java/org/apache/hugegraph/api/job/GremlinAPI.java](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aHVnZWdyYXBoLWFwaS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL2FwaS9qb2IvR3JlbWxpbkFQSS5qYXZh) | `50.72% <ø> (ø)` | |
| [...egraph/backend/store/cassandra/CassandraShard.java](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aHVnZWdyYXBoLWNhc3NhbmRyYS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL2JhY2tlbmQvc3RvcmUvY2Fzc2FuZHJhL0Nhc3NhbmRyYVNoYXJkLmphdmE=) | `0.00% <ø> (-54.55%)` | :arrow_down: |
| [...va/org/apache/hugegraph/analyzer/AnsjAnalyzer.java](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aHVnZWdyYXBoLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1Z2VncmFwaC9hbmFseXplci9BbnNqQW5hbHl6ZXIuamF2YQ==) | `0.00% <ø> (-60.87%)` | :arrow_down: |
| [...a/org/apache/hugegraph/analyzer/HanLPAnalyzer.java](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aHVnZWdyYXBoLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1Z2VncmFwaC9hbmFseXplci9IYW5MUEFuYWx5emVyLmphdmE=) | `0.00% <ø> (-68.30%)` | :arrow_down: |
| [...java/org/apache/hugegraph/analyzer/IKAnalyzer.java](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aHVnZWdyYXBoLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1Z2VncmFwaC9hbmFseXplci9JS0FuYWx5emVyLmphdmE=) | `71.42% <ø> (ø)` | |
| [...a/org/apache/hugegraph/analyzer/JcsegAnalyzer.java](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aHVnZWdyYXBoLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1Z2VncmFwaC9hbmFseXplci9KY3NlZ0FuYWx5emVyLmphdmE=) | `0.00% <ø> (-78.95%)` | :arrow_down: |
| [...a/org/apache/hugegraph/analyzer/JiebaAnalyzer.java](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aHVnZWdyYXBoLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1Z2VncmFwaC9hbmFseXplci9KaWViYUFuYWx5emVyLmphdmE=) | `41.66% <ø> (-41.67%)` | :arrow_down: |
| [...org/apache/hugegraph/analyzer/MMSeg4JAnalyzer.java](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aHVnZWdyYXBoLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1Z2VncmFwaC9hbmFseXplci9NTVNlZzRKQW5hbHl6ZXIuamF2YQ==) | `0.00% <ø> (-65.22%)` | :arrow_down: |
| [...org/apache/hugegraph/analyzer/SmartCNAnalyzer.java](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aHVnZWdyYXBoLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1Z2VncmFwaC9hbmFseXplci9TbWFydENOQW5hbHl6ZXIuamF2YQ==) | `0.00% <ø> (-85.72%)` | :arrow_down: |
| [...che/hugegraph/backend/id/SnowflakeIdGenerator.java](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aHVnZWdyYXBoLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1Z2VncmFwaC9iYWNrZW5kL2lkL1Nub3dmbGFrZUlkR2VuZXJhdG9yLmphdmE=) | `69.49% <ø> (ø)` | |
| ... and [186 more](https://codecov.io/gh/apache/incubator-hugegraph/pull/2095?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1122677331
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -137,18 +129,23 @@ public static byte[] compress(String value) {
}
public static byte[] compress(String value, float bufferRatio) {
- BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE,
- bufferRatio);
- return buf.bytes();
+ try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
+ return buf.bytes();
+ } catch (IOException e) {
Review Comment:
the IOException throws by `LZ4Util.compress/decompress` & we just auto release the `BytesBuffer` when it used up, so if we don't catch exception, we need to throws out? (so as the `decompress`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin merged pull request #2095: refact: use standard UTF-8 charset & enhance CI configs
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin merged PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] javeme commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1083234806
##########
hugegraph-cassandra/src/main/java/org/apache/hugegraph/backend/store/cassandra/CassandraShard.java:
##########
@@ -56,12 +56,13 @@
* CassandraShard is used for cassandra scanning operations.
* Each shard represents a range of tokens for a node.
* Reading data from a given shard does not cross multiple nodes.
+ * <p>
* Refer to AbstractColumnFamilyInputFormat from:
* <a href="https://github.com/2013Commons/hive-cassandra/">...</a>
*/
public class CassandraShard {
- // The minimal shard size should >= 1M to prevent too many number of shards
+ /** The minimal shard size should >= 1M to prevent too many number of shards */
Review Comment:
`/**` => `/*`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1123323435
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -137,18 +129,23 @@ public static byte[] compress(String value) {
}
public static byte[] compress(String value, float bufferRatio) {
- BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE,
- bufferRatio);
- return buf.bytes();
+ try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
+ return buf.bytes();
+ } catch (IOException e) {
Review Comment:
> yes the BytesBuffer don't need to close, it's just a memory buffer
OK, my previous consideration is that if the **buffer** is large(like compress a big file), we should recycle it manually in time to reduce the pressure of GC & memory usage, but lack enough validation, so back off for now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1123097675
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -137,18 +129,23 @@ public static byte[] compress(String value) {
}
public static byte[] compress(String value, float bufferRatio) {
- BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE,
- bufferRatio);
- return buf.bytes();
+ try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
+ return buf.bytes();
+ } catch (IOException e) {
Review Comment:
> yes just remove the catch, since the LZ4Util.compress already transfer IOException to BackendException
so shall we close the `ByteBuffer` when it used up? if we remove catch with `try(xx)`, we need throws the exception?
It means we just ignore the `ByteBuffer` close?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] javeme commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1123076734
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -137,26 +127,30 @@ public static byte[] compress(String value) {
}
public static byte[] compress(String value, float bufferRatio) {
- BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE,
- bufferRatio);
- return buf.bytes();
+ try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
+ return buf.bytes();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
Review Comment:
wrap a message for the re-throw, like "Failed to compress xx"
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -137,18 +129,23 @@ public static byte[] compress(String value) {
}
public static byte[] compress(String value, float bufferRatio) {
- BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE,
- bufferRatio);
- return buf.bytes();
+ try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
+ return buf.bytes();
+ } catch (IOException e) {
Review Comment:
yes just remove the catch, since the LZ4Util.compress already transfer IOException to BackendException
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] zyxxoo commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by GitBox <gi...@apache.org>.
zyxxoo commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1081532122
##########
hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/HugeScriptTraversal.java:
##########
@@ -32,6 +32,7 @@
import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal;
/**
+ * TODO: lack LICENSE reference
Review Comment:
look like "https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ScriptTraversal.java"
should i add APV2 to the annotation? we license and the origin license is same
##########
hugegraph-core/src/main/java/org/apache/hugegraph/analyzer/AnsjAnalyzer.java:
##########
@@ -32,6 +32,7 @@
import com.google.common.collect.ImmutableList;
/**
+ * TODO: ensure the refer is valid & clear
Review Comment:
the refer can replace with https://github.com/NLPchina/ansj_seg/wiki
##########
hugegraph-core/src/main/java/org/apache/hugegraph/analyzer/JcsegAnalyzer.java:
##########
@@ -33,6 +33,7 @@
import com.google.common.collect.ImmutableList;
/**
+ * TODO: ensure the refer is valid & clear
Review Comment:
replace with https://github.com/lionsoul2014/jcseg
##########
hugegraph-core/src/main/java/org/apache/hugegraph/type/Nameable.java:
##########
@@ -18,6 +18,7 @@
package org.apache.hugegraph.type;
/**
+ * TODO: lack license header & LICENSE reference
Review Comment:
https://github.com/JanusGraph/janusgraph/blob/master/janusgraph-core/src/main/java/org/janusgraph/core/Namifiable.java
ditto, should I Change the license to janusgraph license?should I modify the license?
##########
hugegraph-core/src/main/java/org/apache/hugegraph/analyzer/SmartCNAnalyzer.java:
##########
@@ -29,6 +29,7 @@
import org.apache.hugegraph.util.InsertionOrderUtil;
/**
+ * TODO: ensure the refer is valid & clear
* Reference from https://my.oschina.net/apdplat/blog/412921
*/
public class SmartCNAnalyzer implements Analyzer {
Review Comment:
replace with https://lucene.apache.org/core/8_11_2/analyzers-smartcn/index.html
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -17,19 +17,22 @@
package org.apache.hugegraph.util;
-import java.io.UnsupportedEncodingException;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import java.io.IOException;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.Base64;
import java.util.UUID;
-import org.mindrot.jbcrypt.BCrypt;
-
import org.apache.hugegraph.HugeException;
import org.apache.hugegraph.backend.serializer.BytesBuffer;
+import org.mindrot.jbcrypt.BCrypt;
+
import com.google.common.base.CharMatcher;
/**
+ * TODO: lack license header & LICENSE reference
Review Comment:
https://github.com/JanusGraph/janusgraph/blob/master/janusgraph-core/src/main/java/org/janusgraph/util/encoding/StringEncoding.java
The janusgraph also is APV2 license, should i keep the janusgraph origin license? should i modify our license file?
##########
hugegraph-core/src/main/java/org/apache/hugegraph/analyzer/IKAnalyzer.java:
##########
@@ -30,6 +30,7 @@
import com.google.common.collect.ImmutableList;
/**
+ * TODO: ensure the refer is valid & clear
Review Comment:
the reference replace with https://github.com/yyn0210/IKAnalyzer
##########
hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/SnowflakeIdGenerator.java:
##########
@@ -94,7 +94,8 @@ public Id generate(HugeVertex vertex) {
return this.generate();
}
- /*
+ /**
+ * TODO: lack LICENSE reference & seems need move to header
Review Comment:
pr 2093 fix
##########
hugegraph-core/src/main/java/org/apache/hugegraph/analyzer/JiebaAnalyzer.java:
##########
@@ -27,6 +27,7 @@
import com.huaban.analysis.jieba.SegToken;
/**
+ * TODO: ensure the refer is valid & clear
Review Comment:
replace with https://github.com/huaban/jieba-analysis
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/GZipUtil.java:
##########
@@ -27,6 +27,7 @@
import org.apache.hugegraph.backend.serializer.BytesBuffer;
/**
+ * TODO: ensure the refer is valid & clear
Review Comment:
ditto, We looked at the code on the blog, maybe we can remove the reference, because the code is a common util
##########
hugegraph-api/src/main/java/org/apache/hugegraph/api/job/GremlinAPI.java:
##########
@@ -150,7 +150,7 @@ public String name() {
return firstLine;
}
- /*
+ /* TODO: ensure the refer is valid
Review Comment:
the refer look like can remove
##########
hugegraph-core/src/main/java/org/apache/hugegraph/analyzer/MMSeg4JAnalyzer.java:
##########
@@ -34,6 +34,7 @@
import com.google.common.collect.ImmutableList;
/**
+ * TODO: ensure the refer is valid & clear
Review Comment:
replace with https://github.com/chenlb/mmseg4j-core
##########
hugegraph-core/src/main/java/org/apache/hugegraph/analyzer/IKAnalyzer.java:
##########
@@ -30,6 +30,7 @@
import com.google.common.collect.ImmutableList;
/**
+ * TODO: ensure the refer is valid & clear
Review Comment:
I will change to download jar
##########
hugegraph-core/src/main/java/org/apache/hugegraph/analyzer/HanLPAnalyzer.java:
##########
@@ -33,6 +33,7 @@
import com.hankcs.hanlp.tokenizer.StandardTokenizer;
/**
+ * TODO: ensure the refer is valid & clear
Review Comment:
replace with https://github.com/hankcs/HanLP/wiki
##########
hugegraph-core/src/main/java/org/apache/hugegraph/type/define/Cardinality.java:
##########
@@ -20,6 +20,7 @@
import org.apache.tinkerpop.gremlin.structure.VertexProperty;
/**
+ * TODO: lack license header & LICENSE reference
Review Comment:
https://github.com/JanusGraph/janusgraph/blob/master/janusgraph-core/src/main/java/org/janusgraph/core/Cardinality.java
https://github.com/JanusGraph/janusgraph/blob/master/janusgraph-core/src/main/java/org/janusgraph/core/Namifiable.java
ditto, should I Change the license to janusgraph license?should I modify the license?
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/CompressUtil.java:
##########
@@ -57,6 +57,7 @@
public final class CompressUtil {
/**
+ * TODO: ensure the refer is valid & clear
Review Comment:
We looked at the code on the blog, maybe we can remove the reference, because the code is a common util
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1101438531
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -92,33 +96,21 @@ public static int getAsciiByteLength(String value) {
}
public static byte[] encode(String value) {
- try {
- return value.getBytes("UTF-8");
- } catch (UnsupportedEncodingException e) {
- throw new HugeException("Failed to encode string", e);
- }
+ return value.getBytes(UTF_8);
Review Comment:
> please don't change the logic
it's recommended by the lint, and here is the differ:
Anyway, we can leave it alone, but we should try to avoid passing the string param for `Charset`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1101438531
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -92,33 +96,21 @@ public static int getAsciiByteLength(String value) {
}
public static byte[] encode(String value) {
- try {
- return value.getBytes("UTF-8");
- } catch (UnsupportedEncodingException e) {
- throw new HugeException("Failed to encode string", e);
- }
+ return value.getBytes(UTF_8);
Review Comment:
> please don't change the logic
it's recommended by the lint, and here is the differ:
Anyway, we can leave it alone, but we should try to avoid passing the string param for `Charset` in future (usage)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] javeme commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1101418717
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -92,33 +96,21 @@ public static int getAsciiByteLength(String value) {
}
public static byte[] encode(String value) {
- try {
- return value.getBytes("UTF-8");
- } catch (UnsupportedEncodingException e) {
- throw new HugeException("Failed to encode string", e);
- }
+ return value.getBytes(UTF_8);
Review Comment:
please don't change the logic
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -137,18 +129,23 @@ public static byte[] compress(String value) {
}
public static byte[] compress(String value, float bufferRatio) {
- BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE,
- bufferRatio);
- return buf.bytes();
+ try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
+ return buf.bytes();
+ } catch (IOException e) {
Review Comment:
seems don't need to catch IOException
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -137,18 +129,23 @@ public static byte[] compress(String value) {
}
public static byte[] compress(String value, float bufferRatio) {
- BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE,
- bufferRatio);
- return buf.bytes();
+ try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
Review Comment:
keep the origin logic, or split into a new commit?
##########
hugegraph-cassandra/src/main/java/org/apache/hugegraph/backend/store/cassandra/CassandraShard.java:
##########
@@ -56,12 +56,13 @@
* CassandraShard is used for cassandra scanning operations.
* Each shard represents a range of tokens for a node.
* Reading data from a given shard does not cross multiple nodes.
+ * <p>
* Refer to AbstractColumnFamilyInputFormat from:
* <a href="https://github.com/2013Commons/hive-cassandra/">...</a>
*/
public class CassandraShard {
- // The minimal shard size should >= 1M to prevent too many number of shards
+ /** The minimal shard size should >= 1M to prevent too many number of shards */
Review Comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1098853204
##########
hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/HugeScriptTraversal.java:
##########
@@ -32,6 +32,7 @@
import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal;
/**
+ * TODO: lack LICENSE reference
Review Comment:
> look like "[apache/tinkerpop@`master`/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ScriptTraversal.java](https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ScriptTraversal.java?rgh-link-date=2023-01-20T12%3A37%3A02Z)" should i add APV2 to the annotation? we license and the origin license is same
no need modify header but should refer in LICENSE & NOTICE, and remove the `TODO` if we fixed it
##########
hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/HugeScriptTraversal.java:
##########
@@ -32,6 +32,7 @@
import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal;
/**
+ * TODO: lack LICENSE reference
Review Comment:
> look like "[apache/tinkerpop@`master`/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ScriptTraversal.java](https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ScriptTraversal.java?rgh-link-date=2023-01-20T12%3A37%3A02Z)" should i add APV2 to the annotation? we license and the origin license is same
no need to modify header but should refer in LICENSE & NOTICE, and remove the `TODO` if we fixed it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] zyxxoo commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "zyxxoo (via GitHub)" <gi...@apache.org>.
zyxxoo commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1081528422
##########
hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/SnowflakeIdGenerator.java:
##########
@@ -94,7 +94,8 @@ public Id generate(HugeVertex vertex) {
return this.generate();
}
- /*
+ /**
+ * TODO: lack LICENSE reference & seems need move to header
Review Comment:
fix in #2093
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1098847654
##########
hugegraph-cassandra/src/main/java/org/apache/hugegraph/backend/store/cassandra/CassandraShard.java:
##########
@@ -56,12 +56,13 @@
* CassandraShard is used for cassandra scanning operations.
* Each shard represents a range of tokens for a node.
* Reading data from a given shard does not cross multiple nodes.
+ * <p>
* Refer to AbstractColumnFamilyInputFormat from:
* <a href="https://github.com/2013Commons/hive-cassandra/">...</a>
*/
public class CassandraShard {
- // The minimal shard size should >= 1M to prevent too many number of shards
+ /** The minimal shard size should >= 1M to prevent too many number of shards */
Review Comment:
> `/**` => `/*`
seems we should use `/** /` for the class property comment (because it will generate doc)
like in `String.java`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1101432776
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -137,18 +129,23 @@ public static byte[] compress(String value) {
}
public static byte[] compress(String value, float bufferRatio) {
- BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE,
- bufferRatio);
- return buf.bytes();
+ try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
Review Comment:
this commit will remove all TODOs after fix them & contains current commit
but we could not merge it now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] javeme commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1123222993
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -137,18 +129,23 @@ public static byte[] compress(String value) {
}
public static byte[] compress(String value, float bufferRatio) {
- BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE,
- bufferRatio);
- return buf.bytes();
+ try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
+ return buf.bytes();
+ } catch (IOException e) {
Review Comment:
yes the BytesBuffer don't need to close, it's just a memory buffer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2095: refact: mark TODO in the license problem code & use standard UTF-8
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2095:
URL: https://github.com/apache/incubator-hugegraph/pull/2095#discussion_r1123323435
##########
hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -137,18 +129,23 @@ public static byte[] compress(String value) {
}
public static byte[] compress(String value, float bufferRatio) {
- BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE,
- bufferRatio);
- return buf.bytes();
+ try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
+ return buf.bytes();
+ } catch (IOException e) {
Review Comment:
> yes the BytesBuffer don't need to close, it's just a memory buffer
OK, my previous consideration is that if the **buffer** is large(like compress a big file), we should recycle it in time to reduce the pressure of GC, but lack enough validation, so back off for now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org