keith-turner commented on code in PR #3417:
URL: https://github.com/apache/accumulo/pull/3417#discussion_r1206152354
##########
core/src/main/java/org/apache/accumulo/core/logging/TabletLogger.java:
##########
@@ -140,7 +140,7 @@ public static void compacting(KeyExtent extent,
CompactionJob job, CompactionCon
}
}
- public static void compacted(KeyExtent extent, CompactionJob job, TabletFile
output) {
+ public static void compacted(KeyExtent extent, CompactionJob job,
AbstractTabletFile<?> output) {
Review Comment:
Could this be StoredTabletFile? Looking at the two places that called it it
seems StoredTabletFile was passed.
##########
server/base/src/main/java/org/apache/accumulo/server/util/ManagerMetadataUtil.java:
##########
@@ -199,10 +199,10 @@ public static void replaceDatafiles(ServerContext
context, KeyExtent extent,
TabletMutator tablet = context.getAmple().mutateTablet(extent);
datafilesToDelete.forEach(tablet::deleteFile);
- scanFiles.forEach(tablet::putScan);
+
scanFiles.stream().map(StoredTabletFile::getTabletFile).forEach(tablet::putScan);
Review Comment:
Wondering if putScan needs an overloaded method, not sure if its ever
updating existing metadata.
##########
core/src/main/java/org/apache/accumulo/core/metadata/StoredTabletFile.java:
##########
@@ -60,6 +65,22 @@ public Text getMetaUpdateDeleteText() {
return new Text(getMetaUpdateDelete());
}
+ public TabletFile getTabletFile() {
+ return tabletFile;
+ }
+
+ public TableId getTableId() {
+ return tabletFile.getTableId();
+ }
+
+ public String getPathStr() {
+ return tabletFile.getPathStr();
+ }
+
+ public Text getMetaInsertText() {
+ return tabletFile.getMetaInsertText();
+ }
Review Comment:
I think this method should be deleted because it normalizes the path. Its
only used in one place and where its used we are updating existing files in the
metadata table, so do not want to normalize.
```suggestion
```
##########
server/base/src/main/java/org/apache/accumulo/server/util/ManagerMetadataUtil.java:
##########
@@ -199,10 +199,10 @@ public static void replaceDatafiles(ServerContext
context, KeyExtent extent,
TabletMutator tablet = context.getAmple().mutateTablet(extent);
datafilesToDelete.forEach(tablet::deleteFile);
- scanFiles.forEach(tablet::putScan);
+
scanFiles.stream().map(StoredTabletFile::getTabletFile).forEach(tablet::putScan);
if (path.isPresent()) {
- tablet.putFile(path.orElseThrow(), size);
+ tablet.putFile(path.orElseThrow().getTabletFile(), size);
Review Comment:
For the putFile method on
[TabletMutator](https://github.com/apache/accumulo/blob/0017fd8949cc1bc7253b8e90313d4fd1b71e2241/server/base/src/main/java/org/apache/accumulo/server/metadata/TabletMutatorBase.java#L84)
I think it would best to add an overloaded version of the method that takes a
StoredTabletFile (keeping the one that takes a TabletFile if needed). Then if
the code has a StoredTabletFile it can pass it. The putFile() method is called
to add new files and to update the DataFileValue for existing files. For the
case of updating existing files need to pass the StoredTabletFile through
instead of converting to the a TabletFile (which normalizes).
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFiles.java:
##########
@@ -219,7 +221,8 @@ void load(List<TabletMetadata> tablets, Files files) {
server = location.getHostAndPort();
}
- Set<TabletFile> loadedFiles = tablet.getLoaded().keySet();
+ Set<TabletFile> loadedFiles = tablet.getLoaded().keySet().stream()
Review Comment:
Before this change, this code it may have been comparing StoredTabletFile to
TabletFile later in the code. I think this was a place that was relying on the
sketchy hashCode and equals situation.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
