You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2023/06/15 20:23:33 UTC
[GitHub] [accumulo] keith-turner opened a new pull request, #3501: avoids creating path object for GC refs
keith-turner opened a new pull request, #3501:
URL: https://github.com/apache/accumulo/pull/3501
In #3500 it was observed that GC spends a lot of time creating Path objects for each Reference. This changes avoids creating Path objects for the GC case.
Currently have tested these changes without issue against tests matching the pattern `*GC*IT` and `*Garb*IT`. Looking into doing some further performance test to see if there is an observable difference.
--
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] dlmarion commented on pull request #3501: avoids creating path object for GC refs
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1599341280
> > The issue is the last line of the method, right? Could we not create a URI from (String, String, String) and then call Path(URI)?
>
> @dlmarion do you think we might be able to optimize the validation instead of avoid it?
You mean like override `getParent()` in a subclass, copy the method contents and change the last line to use Path(URI) ?
--
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 pull request #3501: avoids creating path object for GC refs
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1595216844
> > Is the plan to merge this to 2.1, and omit this change when merging to 3.0 (`-s ours`), in favor of #3502 fixing it there? Or is there something from this that would still be desirable to keep when going forward?
>
> The only thing to merge forward would be the change to `Pair` so maybe that should be a separate PR.
Or just commit the change to Pair, because it's so trivial. If you want it "reviewed", consider this comment my +1 for the Pair changes.
--
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] keith-turner commented on pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1593717571
Tried running the test mentioned by @dtspence on #3500 [accumulo-jmh-test](https://github.com/dtspence/accumulo-jmh-test) with these changes. Seeing a noticeable difference. Against 2.1.0 saw the following for the benchmark.
```
Benchmark Mode Cnt Score Error Units
GarbageCollectorPerformanceIT.benchmarkCandidates avgt 3 0.012 ± 0.005 ms/op
```
With these changes and running the test against a locally installed 2.1.1-SNAPSHOT saw the following.
```
Benchmark Mode Cnt Score Error Units
GarbageCollectorPerformanceIT.benchmarkCandidates avgt 3 0.003 ± 0.003 ms/op
```
--
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 closed pull request #3501: avoids creating path object for GC refs
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii closed pull request #3501: avoids creating path object for GC refs
URL: https://github.com/apache/accumulo/pull/3501
--
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] keith-turner commented on a diff in pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#discussion_r1231539270
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -91,7 +94,8 @@ public class TabletMetadata {
private boolean sawOldPrevEndRow = false;
private Text endRow;
private Location location;
- private Map<StoredTabletFile,DataFileValue> files;
+ private List<Pair<String,String>> rawFiles;
Review Comment:
I was originally using a map (the first entries should be unique coming from the column qualifier), but decided to change that to a list to make it the least cost data structure to record what is in the metadata table.
--
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 #3501: avoids creating path object for GC refs
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#discussion_r1231555461
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -91,7 +94,8 @@ public class TabletMetadata {
private boolean sawOldPrevEndRow = false;
private Text endRow;
private Location location;
- private Map<StoredTabletFile,DataFileValue> files;
+ private List<Pair<String,String>> rawFiles;
Review Comment:
Since there were a few smaller changes that tried to reduce costs, I wonder which had the biggest impact, and which don't really matter. If there are a few that are more readable and don't really matter for performance, it might be a good balance to try to keep the more readable ones. However, if a follow on ticket is created to address this (by perhaps improving StoredTabletFile) in 3.x, we can probably defer any such readability improvements until then.
--
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] keith-turner commented on a diff in pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#discussion_r1231551733
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -70,13 +70,16 @@
import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.TabletColumnFamily;
import org.apache.accumulo.core.tabletserver.log.LogEntry;
import org.apache.accumulo.core.util.HostAndPort;
+import org.apache.accumulo.core.util.Pair;
import org.apache.accumulo.core.util.ServerServices;
import org.apache.hadoop.io.Text;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
+import com.google.common.base.Supplier;
Review Comment:
Yeah that should be the java version. I think my IDE auto-magically did that when I used Suppliers. I changed it in b2a8a32
--
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] dlmarion commented on pull request #3501: avoids creating path object for GC refs
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1594591301
At it's core, Path is a URI. The Path constructors perform validation on the input, then create and set the URI. Except, when you use the constructor Path(URI), that does not occur and does not incur the cost of the validation. If we used the Path constructors (and the validation) when writing to the metadata table, but used Path(URI) when reading from the metadata table, then we could still keep the Path object vs going back to String.
--
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 pull request #3501: avoids creating path object for GC refs
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1601729135
Closing based on last comment because #3509 is merged and the performance numbers look good on that PR. Can re-open to address this as follow-on, if still needed.
--
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] keith-turner commented on a diff in pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#discussion_r1231542776
##########
core/src/main/java/org/apache/accumulo/core/util/Pair.java:
##########
@@ -23,8 +23,8 @@
import java.util.Objects;
public class Pair<A,B> {
- A first;
- B second;
+ final A first;
+ final B second;
Review Comment:
Seems they can be protected, a subclass uses them. I made the change in e78ab35
--
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] cshannon commented on pull request #3501: avoids creating path object for GC refs
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1593775729
@keith-turner - I can do a follow on issue to do lazy validation because I agree with @ctubbsii that keeping the new API is a lot nicer than strings so being able to revert these changes in 3.0 would be nice.
--
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] cshannon commented on pull request #3501: avoids creating path object for GC refs
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1594615296
PR to fix this in main is up: https://github.com/apache/accumulo/pull/3502
--
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] dlmarion commented on pull request #3501: avoids creating path object for GC refs
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1594619115
> > At it's core, Path is a URI. The Path constructors perform validation on the input, then create and set the URI. Except, when you use the constructor Path(URI), that does not occur and does not incur the cost of the validation. If we used the Path constructors (and the validation) when writing to the metadata table, but used Path(URI) when reading from the metadata table, then we could still keep the Path object vs going back to String.
>
> The primary bottleneck does not appear to be he actual creation of the Path(). It turns out it's the call to Path.getParent() that is the main problem which is only done in our validation logic.
Right, so we call Path.getParent(), with calls new Path(String, String, String). Here's Path.getParent:
```
/**
* Returns the parent of a path or null if at root.
* @return the parent of a path or null if at root
*/
public Path getParent() {
String path = uri.getPath();
int lastSlash = path.lastIndexOf('/');
int start = startPositionWithoutWindowsDrive(path);
if ((path.length() == start) || // empty path
(lastSlash == start && path.length() == start+1)) { // at root
return null;
}
String parent;
if (lastSlash==-1) {
parent = CUR_DIR;
} else {
parent = path.substring(0, lastSlash==start?start+1:lastSlash);
}
return new Path(uri.getScheme(), uri.getAuthority(), parent);
}
```
The issue is the last line of the method, right? Could we not create a URI from (String, String, String) and then call Path(URI)?
--
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] keith-turner commented on pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1601167112
> Given the comment above, do we still need the changes in this and https://github.com/apache/accumulo/pull/3502?
I am hoping we do not. I do not plan on merging this and hope to close it w/o merging if the perf of #3509 is sufficient.
--
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 #3501: avoids creating path object for GC refs
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#discussion_r1231558576
##########
core/src/main/java/org/apache/accumulo/core/util/Pair.java:
##########
@@ -23,8 +23,8 @@
import java.util.Objects;
public class Pair<A,B> {
- A first;
- B second;
+ final A first;
+ final B second;
Review Comment:
The subclass should probably be made to call `.getFirst()` and `.getSecond()` instead. It doesn't really matter, I guess. Pair is not public API.
--
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] keith-turner commented on a diff in pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#discussion_r1231562890
##########
core/src/main/java/org/apache/accumulo/core/util/Pair.java:
##########
@@ -23,8 +23,8 @@
import java.util.Objects;
public class Pair<A,B> {
- A first;
- B second;
+ final A first;
+ final B second;
Review Comment:
Updated in 62efb44
--
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] keith-turner commented on pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1599440144
> You mean like override getParent() in a subclass, copy the method contents and change the last line to use Path(URI) ?
I am not sure, I have not looked into optimizing what is there. When I made this PR I was working under the assumption that it was the path object in general that was causing problems. However after making this PR it was discovered it was the way Accumulo using the path object. So I was wondering if we should pursue optimizing the validation instead of avoiding it. If we are going to avoid validating on read in 2.1.2, then I am wondering if we should validate on write in 2.1.2 as proposed in #3504.
--
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] keith-turner commented on pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1595410192
Pushed the pair changes to 2.1 and main in fbb54c371e9d76f87bc10a4dfa5d3cc28b4519dd
--
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 #3501: avoids creating path object for GC refs
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#discussion_r1231511531
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -70,13 +70,16 @@
import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.TabletColumnFamily;
import org.apache.accumulo.core.tabletserver.log.LogEntry;
import org.apache.accumulo.core.util.HostAndPort;
+import org.apache.accumulo.core.util.Pair;
import org.apache.accumulo.core.util.ServerServices;
import org.apache.hadoop.io.Text;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
+import com.google.common.base.Supplier;
Review Comment:
This is almost certainly going to trigger modernizer with a suggestion to use `java.util.function.Supplier`. It is possible to use Guava `Suppliers` with the Java functional API, though. I think we have cases where we do that, so it should be easy to change this.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -506,6 +518,12 @@ public static <E extends Entry<Key,Value>> TabletMetadata convertRow(Iterator<E>
if (buildKeyValueMap) {
te.keyValues = kvBuilder.build();
}
+ te.filesSupplier = Suppliers.memoize(() -> {
+ ImmutableMap.Builder<StoredTabletFile,DataFileValue> fb = ImmutableMap.builder();
+ te.rawFiles.forEach(pair -> fb.put(new StoredTabletFile(pair.getFirst()),
+ new DataFileValue((pair.getSecond()))));
+ return fb.build();
Review Comment:
The Guava immutable types are nice APIs to work with, but for simple immutable collections that are constructed in one small part of the code, it's also pretty easy to use something like `.stream()...collect(Collectors.toUnmodifiableMap())` to avoid more dependence on Guava entirely. This might be easier if using a multimap for the source of the stream.
Here's some other ideas: https://www.baeldung.com/java-stream-immutable-collection
I just think using `.forEach(map.put)` is probably the less readable than `stream.collect(mapcollector)`. It's direct, and simple enough, as a programmer to understand.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -91,7 +94,8 @@ public class TabletMetadata {
private boolean sawOldPrevEndRow = false;
private Text endRow;
private Location location;
- private Map<StoredTabletFile,DataFileValue> files;
+ private List<Pair<String,String>> rawFiles;
Review Comment:
Would a multimap make more sense here?
##########
core/src/main/java/org/apache/accumulo/core/util/Pair.java:
##########
@@ -23,8 +23,8 @@
import java.util.Objects;
public class Pair<A,B> {
- A first;
- B second;
+ final A first;
+ final B second;
Review Comment:
Do you know why these are visible? They could probably be private, too.
--
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 #3501: avoids creating path object for GC refs
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#discussion_r1231555461
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -91,7 +94,8 @@ public class TabletMetadata {
private boolean sawOldPrevEndRow = false;
private Text endRow;
private Location location;
- private Map<StoredTabletFile,DataFileValue> files;
+ private List<Pair<String,String>> rawFiles;
Review Comment:
Since there were a few smaller changes that tried to reduce costs, I wonder which had the biggest impact, and which don't really matter. If there are a few that are more readable and don't really matter, that might be a good balance. Mostly, I'm worried about maintainability. However, if a follow on ticket is created to address this (by perhaps improving StoredTabletFile) in 3.x, we can probably defer any such readability improvements until then.
--
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] keith-turner commented on pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1595403553
> The issue is the last line of the method, right? Could we not create a URI from (String, String, String) and then call Path(URI)?
@dlmarion do you think we might be able to optimize the validation instead of avoid 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3501: avoids creating path object for GC refs
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1600864085
If we merged in #3509 then I would not be opposed to reverting #3502, but I could see an argument either way. Lazy loading and lazy validation helps performance if you don't need to validate but there is always the issue of a mistake being made and you do lose the immediate check on creation so you may cause a runtime exception later. Adding validation to MetadataContraints would help prevent that in #3504 and #3506 but no guarantee future changes wouldn't invalid metadata be passed and not caught until later . It would make the changes in #3504 simpler if we reverted the lazy load as I had to do some extra work to specifically not lazy load to ensure validation.
--
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] keith-turner commented on a diff in pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#discussion_r1231556421
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -506,6 +518,12 @@ public static <E extends Entry<Key,Value>> TabletMetadata convertRow(Iterator<E>
if (buildKeyValueMap) {
te.keyValues = kvBuilder.build();
}
+ te.filesSupplier = Suppliers.memoize(() -> {
+ ImmutableMap.Builder<StoredTabletFile,DataFileValue> fb = ImmutableMap.builder();
+ te.rawFiles.forEach(pair -> fb.put(new StoredTabletFile(pair.getFirst()),
+ new DataFileValue((pair.getSecond()))));
+ return fb.build();
Review Comment:
> I seem to remember there was some resistance to getting rid of ImmutableX.builder()
If the rest of the code could be changed to use something built in with java and have the same or better performance I personally would not have a problem with that. That is out of scope for this PR as you said though.
--
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] dlmarion commented on pull request #3501: avoids creating path object for GC refs
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1599475764
@keith-turner - see #3509
--
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] keith-turner commented on a diff in pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#discussion_r1231536776
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -506,6 +518,12 @@ public static <E extends Entry<Key,Value>> TabletMetadata convertRow(Iterator<E>
if (buildKeyValueMap) {
te.keyValues = kvBuilder.build();
}
+ te.filesSupplier = Suppliers.memoize(() -> {
+ ImmutableMap.Builder<StoredTabletFile,DataFileValue> fb = ImmutableMap.builder();
+ te.rawFiles.forEach(pair -> fb.put(new StoredTabletFile(pair.getFirst()),
+ new DataFileValue((pair.getSecond()))));
+ return fb.build();
Review Comment:
For this case the rest of the code is using immutable map builders and those make sense in the other parts of the code. For this code it could use stream, but personally I would rather keep it consistent with the code in the immediate vicinity.
--
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] cshannon commented on pull request #3501: avoids creating path object for GC refs
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1594595272
> At it's core, Path is a URI. The Path constructors perform validation on the input, then create and set the URI. Except, when you use the constructor Path(URI), that does not occur and does not incur the cost of the validation. If we used the Path constructors (and the validation) when writing to the metadata table, but used Path(URI) when reading from the metadata table, then we could still keep the Path object vs going back to String.
The primary bottleneck does not appear to be he actual creation of the Path(). It turns out it's the call to Path.getParent() that is the main problem which is only done in our validation logic.
--
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] dlmarion commented on pull request #3501: avoids creating path object for GC refs
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1600834009
> The primary bottleneck does not appear to be he actual creation of the Path(). It turns out it's the call to Path.getParent() that is the main problem which is only done in our validation logic.
I removed the calls to `Path.getParent()` in #3509 . Given the comment above, do we still need the changes in this and #3502?
--
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] dlmarion commented on pull request #3501: avoids creating path object for GC refs
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1599376837
https://github.com/apache/accumulo/blob/b759a14912b13bade86660c8ba266aec192ab82d/core/src/main/java/org/apache/accumulo/core/metadata/TabletFile.java#L61-L78
Seems like we could just change the validation code to operate on the `metaPath` constructor argument differently. Maybe instead of calling `Path.getParent()`, just use `metaPath.toURI()` and parse the path?
--
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] cshannon commented on pull request #3501: avoids creating path object for GC refs
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1594516801
I added a PR [accumulo-jmh-test](https://github.com/dtspence/accumulo-jmh-test/pull/1) so it can now run on the command line.
--
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 #3501: avoids creating path object for GC refs
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#discussion_r1231552393
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -506,6 +518,12 @@ public static <E extends Entry<Key,Value>> TabletMetadata convertRow(Iterator<E>
if (buildKeyValueMap) {
te.keyValues = kvBuilder.build();
}
+ te.filesSupplier = Suppliers.memoize(() -> {
+ ImmutableMap.Builder<StoredTabletFile,DataFileValue> fb = ImmutableMap.builder();
+ te.rawFiles.forEach(pair -> fb.put(new StoredTabletFile(pair.getFirst()),
+ new DataFileValue((pair.getSecond()))));
+ return fb.build();
Review Comment:
For what it's worth, I think the other code could change, too. I seem to remember there was some resistance to getting rid of `ImmutableX.builder()` code when I last did a pass through the code to replace our use of immutable Guava types with builtin immutables. The builders are nice. Even if we keep them, I think they could be expressed as a custom stream Collector. But that change would be out of scope of this PR.
One other consideration we have to be careful about is checking for null values `.contains(null)` fails on built-in Java immutables, like that returned from `List.of()`, `Set.of()`, etc., instead of just always returning false, which would make more sense. I don't think Guava has the same issue. And, I know `Collections.unmodifiableX()` does not suffer from the same 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1593731225
> The StoredTabletFile is still a somewhat nicer API than going back to using String again (especially when it's a collection of pairs of strings, which gets quite confusing). If the problem is that StoredTabletFile is doing unnecessary validation, can we have a specific kind of TabletFile that is lighter weight, but still more expressive than strings?
I looked into refactoring StoredTabletFile first and decided against that as I could not see an easy way to do it. I think the changes @cshannon has made in 3.0 branch would be a prereq for doing this refactoring.
--
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] cshannon commented on pull request #3501: avoids creating path object for GC refs
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1595201163
> Is the plan to merge this to 2.1, and omit this change when merging to 3.0 (`-s ours`), in favor of #3502 fixing it there? Or is there something from this that would still be desirable to keep when going forward?
The only thing to merge forward would be the change to `Pair` so maybe that should be a separate PR.
--
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] keith-turner commented on pull request #3501: avoids creating path object for GC refs
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1595256028
I think I will pull the pair changes out as their own commit push merge them and as @ctubbsii mentioned they are reviewed so I will not do another PR.
--
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 pull request #3501: avoids creating path object for GC refs
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#issuecomment-1595157815
Is the plan to merge this to 2.1, and omit this change when merging to 3.0 (`-s ours`), in favor of #3502 fixing it there? Or is there something from this that would still be desirable to keep when going forward?
--
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