You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/07/25 19:16:54 UTC
[GitHub] [accumulo] milleruntime opened a new pull request, #2826: Create new method in FastFormat for FATE
milleruntime opened a new pull request, #2826:
URL: https://github.com/apache/accumulo/pull/2826
* Replace String.format() with new method in FastFormat that should be faster
* Modify FateTxId.formatTid to use new method that should be equivalent
* Replace uses of Long.toHexString in Utils with FateTxId.formatTid to
improve logging
* Add new tests to FastFormatTest
* Supports #2495
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime merged pull request #2826: Create new method in FastFormat for FATE
Posted by GitBox <gi...@apache.org>.
milleruntime merged PR #2826:
URL: https://github.com/apache/accumulo/pull/2826
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2826: Create new method in FastFormat for FATE
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2826:
URL: https://github.com/apache/accumulo/pull/2826#issuecomment-1197088567
Output in the logs:
<pre>
manager_ip-10-113-14-231.log:2022-07-27T13:33:07,055 [fate.Fate] INFO : Seeding FATE[1a6f56ffdb22da4e] TABLE_COMPACT Compact table (1) with config []
manager_ip-10-113-14-231.log:2022-07-27T13:33:07,068 [zookeeper.DistributedReadWriteLock] INFO : Added lock entry 0 userData 1a6f56ffdb22da4e lockType READ
manager_ip-10-113-14-231.log:2022-07-27T13:33:07,069 [tableOps.Utils] INFO : namespace +default FATE[1a6f56ffdb22da4e] locked for read operation: COMPACT
manager_ip-10-113-14-231.log:2022-07-27T13:33:07,073 [zookeeper.DistributedReadWriteLock] INFO : Added lock entry 0 userData 1a6f56ffdb22da4e lockType READ
manager_ip-10-113-14-231.log:2022-07-27T13:33:07,074 [tableOps.Utils] INFO : table 1 FATE[1a6f56ffdb22da4e] locked for read operation: COMPACT
</pre>
matches the output from `fate -summary`:
<pre>
Running txn_id Status Command Step (top) locks held:(table id, name) locks waiting:(table id, name)
0:00:17 1a6f56ffdb22da4e IN_PROGRESS CompactRange CompactionDriver held:[R:(+default,ns:), R:(1,t:ci)] waiting:[]
</pre>
I think the only class still missing the prefix is `DistributedReadWriteLock` but I looked at that and it would be pretty tricky.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2826: Create new method in FastFormat for FATE
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2826:
URL: https://github.com/apache/accumulo/pull/2826#issuecomment-1195777534
All ITs are passing.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2826: Create new method in FastFormat for FATE
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2826:
URL: https://github.com/apache/accumulo/pull/2826#discussion_r929284611
##########
core/src/test/java/org/apache/accumulo/core/util/FastFormatTest.java:
##########
@@ -117,4 +118,30 @@ public void testArrayOutOfBounds() {
assertThrows(ArrayIndexOutOfBoundsException.class,
() -> FastFormat.toZeroPaddedString(str, 4, 64L, 4, 16, new byte[] {'P'}));
}
+
+ @Test
+ public void testHexString() {
Review Comment:
Would it be of benefit to add some assertions that this behaves the same as `String.format`?
e.g.
`assertEquals(String.format("%016x", 64L), FastFormat.toHexString(64L));`
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2826: Create new method in FastFormat for FATE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2826:
URL: https://github.com/apache/accumulo/pull/2826#discussion_r929301864
##########
core/src/main/java/org/apache/accumulo/fate/FateTxId.java:
##########
@@ -54,7 +56,9 @@ public static long fromString(String fmtTid) {
*/
public static String formatTid(long tid) {
// do not change how this formats without considering implications for persistence
- return String.format("%s%016x%s", PREFIX, tid, SUFFIX);
+ // original format: String.format("%s%016x%s", PREFIX, tid, SUFFIX);
+ // Since 2.1, this format was replaced with the faster version below
+ return FastFormat.toHexString(PREFIX, tid, SUFFIX);
Review Comment:
Is this a change in format, or just a change in the method called to do the formatting? If it's the former, I'm curious what's different in the new format. If it's the latter, I'm not sure this comment about the original vs. the replacement implementation is very helpful, and it might be misleading.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a diff in pull request #2826: Create new method in FastFormat for FATE
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2826:
URL: https://github.com/apache/accumulo/pull/2826#discussion_r929852006
##########
core/src/main/java/org/apache/accumulo/fate/FateTxId.java:
##########
@@ -54,7 +56,9 @@ public static long fromString(String fmtTid) {
*/
public static String formatTid(long tid) {
// do not change how this formats without considering implications for persistence
- return String.format("%s%016x%s", PREFIX, tid, SUFFIX);
+ // original format: String.format("%s%016x%s", PREFIX, tid, SUFFIX);
+ // Since 2.1, this format was replaced with the faster version below
+ return FastFormat.toHexString(PREFIX, tid, SUFFIX);
Review Comment:
> I'm not sure this comment about the original vs. the replacement implementation is very helpful, and it might be misleading.
My goal was to keep the same format just improve performance. Assuming I didn't screw it up, then I think you are right that the comment could be misleading.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org