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 2021/09/08 17:23:37 UTC
[GitHub] [accumulo-testing] DomGarguilo opened a new pull request #150: Refactor out Non API code
DomGarguilo opened a new pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150
Fixes #53
Refactored out non-api code.
In most cases the imports were just being used to grab a String e.g. a class name. For those cases I just hardcoded a `final String` to use in their place.
--
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-testing] EdColeman commented on a change in pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150#discussion_r705620718
##########
File path: src/main/java/org/apache/accumulo/testing/ingest/VerifyIngest.java
##########
@@ -59,6 +59,9 @@ public static void main(String[] args) throws Exception {
Opts opts = new Opts();
opts.parseArgs(VerifyIngest.class.getName(), args);
try (AccumuloClient client = Accumulo.newClient().from(opts.getClientProps()).build()) {
+ if (opts.trace) {
+ throw new RuntimeException("Trace is enabled but not currently supported");
Review comment:
could use UnsupportedOperationException
--
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-testing] DomGarguilo commented on pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150#issuecomment-915429992
I am still looking into how FastFormat might be refactored out. I know that String.format is a lot slower so wanted to get some feedback on if that is acceptable to use in this case or if something else should be done such as creating a method to reproduce the functionality of FastFormat.
--
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-testing] DomGarguilo merged pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
DomGarguilo merged pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150
--
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-testing] DomGarguilo commented on a change in pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150#discussion_r704655504
##########
File path: src/main/java/org/apache/accumulo/testing/performance/tests/HighSplitCreationPT.java
##########
@@ -36,6 +35,7 @@
private static final int ONE_SECOND = 1000;
private static final String TABLE_NAME = "highSplitCreation";
private static final String METADATA_TABLE_SPLITS = "123456789abcde";
+ private static final String METADATA_TABLE_NAME = "accumulo.metadata";
Review comment:
> I think this is fine but we could probably put the names in a shared place of constants and reuse them.
Good idea. Do you know of a good place for these? @milleruntime
--
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-testing] DomGarguilo commented on a change in pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150#discussion_r704713311
##########
File path: contrib/import-control.xml
##########
@@ -37,18 +37,6 @@
<allow pkg="org.apache.accumulo.core.conf"/>
<allow class="org.apache.accumulo.core.util.threads.ThreadPools"/>
- <!-- TODO refactor code to remove the following exceptions -->
- <allow class="org.apache.accumulo.core.metadata.MetadataTable"/>
- <allow class="org.apache.accumulo.core.replication.ReplicationTable"/>
- <allow class="org.apache.accumulo.core.spi.scan.HintScanPrioritizer"/>
- <allow class="org.apache.accumulo.core.clientImpl.TabletServerBatchWriter"/>
- <allow class="org.apache.accumulo.core.util.FastFormat"/>
- <allow class="org.apache.accumulo.core.util.Pair"/>
- <allow class="org.apache.accumulo.core.trace.Trace"/>
Review comment:
As is, there are commented out usages of Trace in [VerifyIngest.java](https://github.com/apache/accumulo-testing/blob/fbaf16dc2c065eaab690d8d7abd75750d5d0425c/src/main/java/org/apache/accumulo/testing/ingest/VerifyIngest.java#L65). I'm thinking that either the comments should be removed along with the `<allow/>` tag or, both should be kept. For now I'll remove both and change it back if it seems like it should be.
--
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-testing] DomGarguilo commented on pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150#issuecomment-915501882
> The easiest solution would be to just copy the class into testing since I doubt it will be changing any time soon.
I went ahead and did that. All of the exceptions should be accounted 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo-testing] Manno15 commented on a change in pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150#discussion_r704661503
##########
File path: src/main/java/org/apache/accumulo/testing/performance/tests/HighSplitCreationPT.java
##########
@@ -36,6 +35,7 @@
private static final int ONE_SECOND = 1000;
private static final String TABLE_NAME = "highSplitCreation";
private static final String METADATA_TABLE_SPLITS = "123456789abcde";
+ private static final String METADATA_TABLE_NAME = "accumulo.metadata";
Review comment:
TestProps.java seems like a good location.
--
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-testing] DomGarguilo commented on a change in pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150#discussion_r704655504
##########
File path: src/main/java/org/apache/accumulo/testing/performance/tests/HighSplitCreationPT.java
##########
@@ -36,6 +35,7 @@
private static final int ONE_SECOND = 1000;
private static final String TABLE_NAME = "highSplitCreation";
private static final String METADATA_TABLE_SPLITS = "123456789abcde";
+ private static final String METADATA_TABLE_NAME = "accumulo.metadata";
Review comment:
> I think this is fine but we could probably put the names in a shared place of constants and reuse them.
Good idea. Do you know of what a good place for these might be? @milleruntime
--
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-testing] milleruntime commented on a change in pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150#discussion_r704646092
##########
File path: src/main/java/org/apache/accumulo/testing/performance/tests/HighSplitCreationPT.java
##########
@@ -36,6 +35,7 @@
private static final int ONE_SECOND = 1000;
private static final String TABLE_NAME = "highSplitCreation";
private static final String METADATA_TABLE_SPLITS = "123456789abcde";
+ private static final String METADATA_TABLE_NAME = "accumulo.metadata";
Review comment:
I think this is fine but we could probably put the names in a shared place of constants and reuse them.
##########
File path: src/main/java/org/apache/accumulo/testing/merkle/MerkleTree.java
##########
@@ -21,7 +21,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.accumulo.core.util.Pair;
+import org.apache.commons.math3.util.Pair;
Review comment:
Nice find!
--
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-testing] milleruntime commented on a change in pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150#discussion_r704734652
##########
File path: contrib/import-control.xml
##########
@@ -37,18 +37,6 @@
<allow pkg="org.apache.accumulo.core.conf"/>
<allow class="org.apache.accumulo.core.util.threads.ThreadPools"/>
- <!-- TODO refactor code to remove the following exceptions -->
- <allow class="org.apache.accumulo.core.metadata.MetadataTable"/>
- <allow class="org.apache.accumulo.core.replication.ReplicationTable"/>
- <allow class="org.apache.accumulo.core.spi.scan.HintScanPrioritizer"/>
- <allow class="org.apache.accumulo.core.clientImpl.TabletServerBatchWriter"/>
- <allow class="org.apache.accumulo.core.util.FastFormat"/>
- <allow class="org.apache.accumulo.core.util.Pair"/>
- <allow class="org.apache.accumulo.core.trace.Trace"/>
Review comment:
My guess is that was commented out because either it wasn't working or the Trace code was in flux (it still is). Since there is an option to use the Trace in VerifyIngest, we could throw an exception stating it is currently not supported.
--
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-testing] DomGarguilo commented on a change in pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150#discussion_r705618408
##########
File path: contrib/import-control.xml
##########
@@ -37,18 +37,6 @@
<allow pkg="org.apache.accumulo.core.conf"/>
<allow class="org.apache.accumulo.core.util.threads.ThreadPools"/>
- <!-- TODO refactor code to remove the following exceptions -->
- <allow class="org.apache.accumulo.core.metadata.MetadataTable"/>
- <allow class="org.apache.accumulo.core.replication.ReplicationTable"/>
- <allow class="org.apache.accumulo.core.spi.scan.HintScanPrioritizer"/>
- <allow class="org.apache.accumulo.core.clientImpl.TabletServerBatchWriter"/>
- <allow class="org.apache.accumulo.core.util.FastFormat"/>
- <allow class="org.apache.accumulo.core.util.Pair"/>
- <allow class="org.apache.accumulo.core.trace.Trace"/>
Review comment:
Something like 96ff1ac? @milleruntime
--
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-testing] DomGarguilo commented on a change in pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150#discussion_r705623687
##########
File path: src/main/java/org/apache/accumulo/testing/ingest/VerifyIngest.java
##########
@@ -59,6 +59,9 @@ public static void main(String[] args) throws Exception {
Opts opts = new Opts();
opts.parseArgs(VerifyIngest.class.getName(), args);
try (AccumuloClient client = Accumulo.newClient().from(opts.getClientProps()).build()) {
+ if (opts.trace) {
+ throw new RuntimeException("Trace is enabled but not currently supported");
Review comment:
Good idea. Addressed in 02248ad
--
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-testing] DomGarguilo commented on a change in pull request #150: Refactor out Non API code
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #150:
URL: https://github.com/apache/accumulo-testing/pull/150#discussion_r704702963
##########
File path: src/main/java/org/apache/accumulo/testing/performance/tests/HighSplitCreationPT.java
##########
@@ -36,6 +35,7 @@
private static final int ONE_SECOND = 1000;
private static final String TABLE_NAME = "highSplitCreation";
private static final String METADATA_TABLE_SPLITS = "123456789abcde";
+ private static final String METADATA_TABLE_NAME = "accumulo.metadata";
Review comment:
Addressed in c8b6cf4
--
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