Create StoredTabletFile to use in different situations by milleruntime · Pull Request #1519 · apache/accumulo

milleruntime · 2020-02-18T18:04:53Z

Created methods for read/insert/updateDel and have them called
throughout the code for the appropriate situation
Throw exception if we try to get metadata for update or delete and one
does not exist
Call inserted() method on TabletFile to update metadata entry when a
file is new and inserted
Drop FileUtil.toPathStrings() and use object collections instead
Replace String path with TabletFile in FileUtil

core/src/main/java/org/apache/accumulo/core/metadata/TabletFile.java

ctubbsii · 2020-02-18T23:10:58Z

server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java

    tablet.putTime(time);
    estSizes.forEach(tablet::putFile);

+    // TODO check to see if we need to delete.. putBulkFile does an insert


Should resolve this before merge, or create follow-on task.

I think this is OK since it is only called by the 2 bulk import methods. And each has a corresponding CleanupBulkImport FATE operations which callsMetadataTableUtil.removeBulkLoadEntries()

core/src/main/java/org/apache/accumulo/core/metadata/TabletFile.java

* Created methods for read/insert/updateDel and have them called throughout the code for the appropriate situation * Throw exception if we try to get metadata for update or delete and one does not exist * Call inserted() method on TabletFile to update metadata entry when a file is new and inserted * Drop FileUtil.toPathStrings() and use object collections instead * Replace String path with TabletFile in FileUtil

Co-Authored-By: Keith Turner <kturner@apache.org>

ctubbsii · 2020-02-24T21:25:32Z

@milleruntime I can't tell if you're still working on this or if it's ready for re-review. If you want a re-review, feel free to tag me.

milleruntime · 2020-02-25T13:15:34Z

@milleruntime I can't tell if you're still working on this or if it's ready for re-review. If you want a re-review, feel free to tag me.

I am still working on this... I started another branch since these changes are involved. I will merge it with this branch and ping you when its ready.

* New class that extends TabletFile for situations where we need to update or delete a TabletFile or read it directly from metadata

milleruntime · 2020-02-25T19:42:54Z

@ctubbsii I made the updates you and @keith-turner suggested so this PR can be reviewed. I know of at least 1 IT that is failing, SplitRecoveryIT that i am still trying to fix.

keith-turner

The change to add the more specific type is nice.

milleruntime · 2020-02-26T17:08:31Z

@keith-turner I added a TODO to take a close look at the compaction code here. I noticed a difference in the order metadata is updated between minor and major compactions. During minor compactions, we update the metadata table first and then update the Tablet data in memory. Major compactions update the tablet data in memory first (with the tablet lock) then outside the lock wait for scans and then finally update the metadata table. Do you know why the metadata isn't updated first during major compaction?

keith-turner · 2020-02-26T19:21:39Z

https://issues.apache.org/jira/browse/ACCUMULO-18

milleruntime · 2020-02-26T21:56:54Z

Seeing a failure in BadIteratorMincIT:

2020-02-26 16:13:35,689 [tablet.Compactor] ERROR: null
java.lang.NullPointerException
        at org.apache.accumulo.test.functional.BadIterator.hasTop(BadIterator.java:36)
        at org.apache.accumulo.tserver.tablet.Compactor.compactLocalityGroup(Compactor.java:380)
        at org.apache.accumulo.tserver.tablet.Compactor.call(Compactor.java:231)
        at org.apache.accumulo.tserver.tablet.MinorCompactor.call(MinorCompactor.java:128)
        at org.apache.accumulo.tserver.tablet.Tablet.minorCompact(Tablet.java:820)
        at org.apache.accumulo.tserver.tablet.MinorCompactionTask.run(MinorCompactionTask.java:94)
        at org.apache.accumulo.fate.util.LoggingRunnable.run(LoggingRunnable.java:37)
        at org.apache.htrace.wrappers.TraceRunnable.run(TraceRunnable.java:57)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
        at org.apache.accumulo.fate.util.LoggingRunnable.run(LoggingRunnable.java:37)
        at java.base/java.lang.Thread.run(Thread.java:834)

ctubbsii

+1 overall; I like the TabletFile superclass / StoredTabletFile subclass mechanism.

core/src/main/java/org/apache/accumulo/core/clientImpl/Translator.java

core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java

* Replace remaining uses with Path objects and removed unnecessary wrapping of intermediate objects * Replaced with more specific types introduced in apache#1519 and apache#1501 * This class was confusing and ambiguous

* Drop FileRef class * Replace remaining uses with Path objects and removed unnecessary wrapping of intermediate objects * Replaced with more specific types introduced in #1519 and #1501 * This class was confusing and ambiguous * PR Feedback for #1726 (milleruntime#14) * Remove unneeded Ample interface (and unneeded 'public' keywords) * Update Ample.deleteBulkFile to use TabletFile (currently not used by any code, but presumably, this will make it easier to update existing code to use this method when it is migrated to use Ample) * Use HashSet instead of HashMap in CopyFailed.java * Remove unnecessary toString in CopyFailed.java Co-authored-by: Christopher Tubbs <ctubbsii@apache.org>

* Revert some changes to FileUtil that used TabletFile. If a tablet is splitting and exceeds the tserver.tablet.split.midpoint.files.max then we will reduce the number of index files using temporary files. These temporary files don't conform to standard TabletFile directories so just use strings in this special case.

* Revert some changes to FileUtil that used TabletFile. If a tablet is splitting and exceeds the tserver.tablet.split.midpoint.files.max then we will reduce the number of index files using temporary files. These temporary files don't conform to standard TabletFile directories so just use strings in this special case. * Closes apache#2977

* Revert some changes to FileUtil that used TabletFile. If a tablet is splitting and exceeds the tserver.tablet.split.midpoint.files.max then we will reduce the number of index files using temporary files. These temporary files don't conform to standard TabletFile directories so just use strings in this special case. * Closes #2977

ctubbsii reviewed Feb 18, 2020

View reviewed changes

keith-turner reviewed Feb 20, 2020

View reviewed changes

ctubbsii reviewed Feb 20, 2020

View reviewed changes

core/src/main/java/org/apache/accumulo/core/metadata/TabletFile.java Outdated Show resolved Hide resolved

milleruntime and others added 3 commits February 21, 2020 09:10

Apply suggestions from code review

be8fe87

Co-Authored-By: Keith Turner <kturner@apache.org>

PR Updates

d80cc3e

milleruntime force-pushed the update-TabletFile branch from def68d4 to d80cc3e Compare February 21, 2020 14:10

Create StoredTabletFile

95b92c5

* New class that extends TabletFile for situations where we need to update or delete a TabletFile or read it directly from metadata

keith-turner approved these changes Feb 26, 2020

View reviewed changes

Fix SplitRecoveryIT

01f7b68

Replace TODO with issue number

850bcd0

ctubbsii approved these changes Feb 26, 2020

View reviewed changes

core/src/main/java/org/apache/accumulo/core/clientImpl/Translator.java Show resolved Hide resolved

core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java Show resolved Hide resolved

milleruntime changed the title ~~Update TabletFile to use getters for different situations~~ Create StoredTabletFile to use in different situations Feb 27, 2020

milleruntime mentioned this pull request Feb 27, 2020

Throw Exception to prevent incorrect metadata #1507

Closed

ctubbsii linked an issue Feb 27, 2020 that may be closed by this pull request

Throw Exception to prevent incorrect metadata #1507

Closed

Fix for minC - prevent checking map when the file is empty

2540af7

milleruntime merged commit 94844b3 into apache:master Feb 27, 2020

milleruntime deleted the update-TabletFile branch February 27, 2020 18:38

milleruntime mentioned this pull request Feb 27, 2020

Investigate empty files in Major Compactions #1535

Closed

keith-turner mentioned this pull request May 28, 2020

ACCUMULO-4419: Change how compression delegation works #140

Closed

milleruntime mentioned this pull request Oct 7, 2020

Drop FileRef class #1726

Merged

ctubbsii added this to the 2.1.0 milestone Jul 12, 2024

Conversation

milleruntime commented Feb 18, 2020

Uh oh!

Uh oh!

ctubbsii Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

milleruntime Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ctubbsii commented Feb 24, 2020

Uh oh!

milleruntime commented Feb 25, 2020

Uh oh!

milleruntime commented Feb 25, 2020

Uh oh!

keith-turner left a comment

Choose a reason for hiding this comment

Uh oh!

milleruntime commented Feb 26, 2020

Uh oh!

keith-turner commented Feb 26, 2020

Uh oh!

milleruntime commented Feb 26, 2020

Uh oh!

ctubbsii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants