Skip to content

Conversation

bharatviswa504
Copy link
Contributor

No description provided.

@bharatviswa504 bharatviswa504 requested a review from arp7 July 16, 2019 00:24
@bharatviswa504 bharatviswa504 self-assigned this Jul 16, 2019
@bharatviswa504
Copy link
Contributor Author

bharatviswa504 commented Jul 16, 2019

One missing thing in this PR is on the restart, we should load up tables with cache, where cleanup policy is NEVER from DB. Will post a fix for that in next commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why we added this policy-specific check here. However it is probably misplaced here. Your original solution to have two cache types with isExist overloaded was probably better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.. we should not have these checks here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Cache doesn't know it is being invoked on flush.. so this check is misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is caller responsibility to call this method after the flush. And right now this is called only after flushing to DB from OzoneManagerDoubleBuffer.

So, if it is NEVER, we will not do the cleanup.

@hadoop-yetus

This comment has been minimized.

@arp7
Copy link
Contributor

arp7 commented Jul 16, 2019

Capturing our offline discussion. We discussed the following:

  1. Populate full cache on process startup (mentioned by Bharat).
  2. Move decision for full lookup into cache implementation. The result will be communicated to TypedTable.
  3. Rename AFTER_FLUSH to MANUAL.

Anything I missed?

@bharatviswa504
Copy link
Contributor Author

bharatviswa504 commented Jul 16, 2019

Capturing our offline discussion. We discussed the following:

  1. Populate full cache on process startup (mentioned by Bharat).
  2. Move decision for full lookup into cache implementation. The result will be communicated to TypedTable.
  3. Rename AFTER_FLUSH to MANUAL.

Anything I missed?

Thank You @arp7 for the offline discussion.
And one more comment by you, when delete of cache entry (like delete volume/bucket), we should delete cache entry, as for full cache, cleanup will not happen.

@hadoop-yetus

This comment has been minimized.

@bharatviswa504
Copy link
Contributor Author

/retest

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 37 Docker mode activated.
_ Prechecks _
+1 dupname 1 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 2 new or modified test files.
_ trunk Compile Tests _
0 mvndep 71 Maven dependency ordering for branch
+1 mvninstall 498 trunk passed
+1 compile 260 trunk passed
+1 checkstyle 75 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 872 branch has no errors when building and testing our client artifacts.
+1 javadoc 162 trunk passed
0 spotbugs 313 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 506 trunk passed
_ Patch Compile Tests _
0 mvndep 37 Maven dependency ordering for patch
+1 mvninstall 445 the patch passed
+1 compile 267 the patch passed
+1 javac 267 the patch passed
-0 checkstyle 39 hadoop-hdds: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 674 patch has no errors when building and testing our client artifacts.
+1 javadoc 163 the patch passed
+1 findbugs 603 the patch passed
_ Other Tests _
+1 unit 271 hadoop-hdds in the patch passed.
-1 unit 1851 hadoop-ozone in the patch failed.
+1 asflicense 54 The patch does not generate ASF License warnings.
7123
Reason Tests
Failed junit tests hadoop.ozone.client.rpc.TestReadRetries
hadoop.ozone.client.rpc.TestOzoneRpcClientWithRatis
hadoop.ozone.client.rpc.TestCloseContainerHandlingByClient
hadoop.ozone.client.rpc.TestCommitWatcher
hadoop.ozone.client.rpc.TestSecureOzoneRpcClient
hadoop.ozone.client.rpc.TestBlockOutputStream
hadoop.ozone.client.rpc.TestOzoneRpcClient
hadoop.ozone.client.rpc.TestOzoneAtRestEncryption
hadoop.ozone.client.rpc.TestOzoneClientRetriesOnException
Subsystem Report/Notes
Docker Client=18.09.7 Server=18.09.7 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/5/artifact/out/Dockerfile
GITHUB PR #1100
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux ffaf70d115f8 4.4.0-138-generic #164-Ubuntu SMP Tue Oct 2 17:16:02 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 85d9111
Default Java 1.8.0_212
checkstyle https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/5/artifact/out/diff-checkstyle-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/5/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/5/testReport/
Max. process+thread count 5337 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/5/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 38 Docker mode activated.
_ Prechecks _
+1 dupname 1 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 2 new or modified test files.
_ trunk Compile Tests _
0 mvndep 65 Maven dependency ordering for branch
+1 mvninstall 495 trunk passed
+1 compile 262 trunk passed
+1 checkstyle 70 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 865 branch has no errors when building and testing our client artifacts.
+1 javadoc 226 trunk passed
0 spotbugs 307 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 491 trunk passed
_ Patch Compile Tests _
0 mvndep 36 Maven dependency ordering for patch
+1 mvninstall 455 the patch passed
+1 compile 276 the patch passed
+1 javac 276 the patch passed
+1 checkstyle 69 the patch passed
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 627 patch has no errors when building and testing our client artifacts.
+1 javadoc 145 the patch passed
+1 findbugs 526 the patch passed
_ Other Tests _
+1 unit 286 hadoop-hdds in the patch passed.
-1 unit 1865 hadoop-ozone in the patch failed.
+1 asflicense 52 The patch does not generate ASF License warnings.
6998
Reason Tests
Failed junit tests hadoop.ozone.client.rpc.TestBCSID
hadoop.ozone.client.rpc.TestOzoneRpcClient
hadoop.ozone.client.rpc.TestOzoneClientRetriesOnException
hadoop.ozone.client.rpc.TestHybridPipelineOnDatanode
hadoop.ozone.ozShell.TestOzoneShell
hadoop.ozone.client.rpc.TestOzoneAtRestEncryption
hadoop.ozone.container.common.statemachine.commandhandler.TestCloseContainerByPipeline
hadoop.ozone.container.common.statemachine.commandhandler.TestCloseContainerHandler
hadoop.ozone.ozShell.TestS3Shell
hadoop.ozone.ozShell.TestOzoneDatanodeShell
hadoop.ozone.client.rpc.TestOzoneRpcClientWithRatis
hadoop.ozone.client.rpc.TestCloseContainerHandlingByClient
Subsystem Report/Notes
Docker Client=18.09.7 Server=18.09.7 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/6/artifact/out/Dockerfile
GITHUB PR #1100
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux 95cb7a610dc8 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 85d9111
Default Java 1.8.0_212
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/6/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/6/testReport/
Max. process+thread count 5390 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/6/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 86 Docker mode activated.
_ Prechecks _
+1 dupname 1 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 2 new or modified test files.
_ trunk Compile Tests _
0 mvndep 26 Maven dependency ordering for branch
+1 mvninstall 519 trunk passed
+1 compile 294 trunk passed
+1 checkstyle 73 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 943 branch has no errors when building and testing our client artifacts.
+1 javadoc 183 trunk passed
0 spotbugs 388 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 630 trunk passed
_ Patch Compile Tests _
0 mvndep 33 Maven dependency ordering for patch
+1 mvninstall 475 the patch passed
+1 compile 296 the patch passed
+1 javac 296 the patch passed
-0 checkstyle 35 hadoop-hdds: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 732 patch has no errors when building and testing our client artifacts.
+1 javadoc 181 the patch passed
+1 findbugs 663 the patch passed
_ Other Tests _
+1 unit 346 hadoop-hdds in the patch passed.
-1 unit 2292 hadoop-ozone in the patch failed.
+1 asflicense 42 The patch does not generate ASF License warnings.
8059
Reason Tests
Failed junit tests hadoop.hdds.scm.pipeline.TestRatisPipelineCreateAndDestory
hadoop.ozone.client.rpc.TestSecureOzoneRpcClient
hadoop.ozone.client.rpc.TestOzoneRpcClientWithRatis
hadoop.ozone.client.rpc.TestReadRetries
hadoop.ozone.TestContainerStateMachineIdempotency
hadoop.ozone.client.rpc.TestHybridPipelineOnDatanode
hadoop.ozone.client.rpc.TestBlockOutputStream
hadoop.ozone.client.rpc.TestOzoneAtRestEncryption
hadoop.ozone.client.rpc.TestCommitWatcher
hadoop.ozone.TestMiniChaosOzoneCluster
hadoop.ozone.client.rpc.TestOzoneRpcClient
Subsystem Report/Notes
Docker Client=18.09.7 Server=18.09.7 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/4/artifact/out/Dockerfile
GITHUB PR #1100
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux 3ffb0bb66273 4.15.0-52-generic #56-Ubuntu SMP Tue Jun 4 22:49:08 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 85d9111
Default Java 1.8.0_212
checkstyle https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/4/artifact/out/diff-checkstyle-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/4/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/4/testReport/
Max. process+thread count 3871 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/4/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 87 Docker mode activated.
_ Prechecks _
+1 dupname 0 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 2 new or modified test files.
_ trunk Compile Tests _
0 mvndep 66 Maven dependency ordering for branch
+1 mvninstall 556 trunk passed
+1 compile 289 trunk passed
+1 checkstyle 78 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 943 branch has no errors when building and testing our client artifacts.
+1 javadoc 180 trunk passed
0 spotbugs 399 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 641 trunk passed
_ Patch Compile Tests _
0 mvndep 32 Maven dependency ordering for patch
+1 mvninstall 481 the patch passed
+1 compile 291 the patch passed
+1 javac 291 the patch passed
-0 checkstyle 41 hadoop-hdds: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 749 patch has no errors when building and testing our client artifacts.
+1 javadoc 185 the patch passed
+1 findbugs 660 the patch passed
_ Other Tests _
+1 unit 346 hadoop-hdds in the patch passed.
-1 unit 3026 hadoop-ozone in the patch failed.
+1 asflicense 41 The patch does not generate ASF License warnings.
8898
Reason Tests
Failed junit tests hadoop.hdds.scm.pipeline.TestRatisPipelineCreateAndDestory
hadoop.ozone.client.rpc.TestSecureOzoneRpcClient
hadoop.ozone.client.rpc.TestOzoneRpcClientWithRatis
hadoop.ozone.om.TestSecureOzoneManager
hadoop.ozone.client.rpc.TestBCSID
hadoop.ozone.client.rpc.TestHybridPipelineOnDatanode
hadoop.ozone.client.rpc.TestBlockOutputStream
hadoop.ozone.om.TestOzoneManagerHA
hadoop.ozone.client.rpc.TestOzoneAtRestEncryption
hadoop.ozone.client.rpc.TestCommitWatcher
hadoop.ozone.container.common.statemachine.commandhandler.TestCloseContainerByPipeline
hadoop.ozone.TestMiniChaosOzoneCluster
Subsystem Report/Notes
Docker Client=18.09.7 Server=18.09.7 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/3/artifact/out/Dockerfile
GITHUB PR #1100
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux 8ad82362a971 4.15.0-52-generic #56-Ubuntu SMP Tue Jun 4 22:49:08 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 85d9111
Default Java 1.8.0_212
checkstyle https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/3/artifact/out/diff-checkstyle-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/3/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/3/testReport/
Max. process+thread count 3569 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/3/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@bharatviswa504
Copy link
Contributor Author

@arp7 Thanks for the review.
I have addressed the review comments.

Copy link
Contributor

@arp7 arp7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Minor suggestions to rename an enum and a function. Looks great otherwise.

@bharatviswa504
Copy link
Contributor Author

Thank You @arp7 for the review.
I have addressed the review comments.

Copy link
Contributor

@arp7 arp7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 pending CI. Thanks @bharatviswa504!

@bharatviswa504
Copy link
Contributor Author

Thank You @arp7 for the review.
I will commit this to the trunk.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 76 Docker mode activated.
_ Prechecks _
+1 dupname 1 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 2 new or modified test files.
_ trunk Compile Tests _
0 mvndep 24 Maven dependency ordering for branch
+1 mvninstall 509 trunk passed
+1 compile 299 trunk passed
+1 checkstyle 80 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 988 branch has no errors when building and testing our client artifacts.
+1 javadoc 184 trunk passed
0 spotbugs 359 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 581 trunk passed
_ Patch Compile Tests _
0 mvndep 72 Maven dependency ordering for patch
+1 mvninstall 479 the patch passed
+1 compile 321 the patch passed
+1 javac 321 the patch passed
+1 checkstyle 83 the patch passed
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 808 patch has no errors when building and testing our client artifacts.
+1 javadoc 202 the patch passed
-1 findbugs 236 hadoop-ozone in the patch failed.
_ Other Tests _
-1 unit 396 hadoop-hdds in the patch failed.
-1 unit 1312 hadoop-ozone in the patch failed.
+1 asflicense 43 The patch does not generate ASF License warnings.
7100
Reason Tests
Failed junit tests hadoop.hdds.scm.container.placement.algorithms.TestContainerPlacementFactory
hadoop.ozone.client.rpc.TestFailureHandlingByClient
hadoop.hdds.scm.pipeline.TestRatisPipelineCreateAndDestory
hadoop.ozone.container.server.TestSecureContainerServer
Subsystem Report/Notes
Docker Client=18.09.7 Server=18.09.7 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/7/artifact/out/Dockerfile
GITHUB PR #1100
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux f3429738ea67 4.15.0-52-generic #56-Ubuntu SMP Tue Jun 4 22:49:08 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 5e6cc6f
Default Java 1.8.0_212
findbugs https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/7/artifact/out/patch-findbugs-hadoop-ozone.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/7/artifact/out/patch-unit-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/7/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/7/testReport/
Max. process+thread count 5378 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1100/7/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@bharatviswa504 bharatviswa504 merged commit 73e6ffc into apache:trunk Jul 18, 2019
@bharatviswa504 bharatviswa504 deleted the HDDS-1802 branch August 1, 2019 02:13
shanthoosh pushed a commit to shanthoosh/hadoop that referenced this pull request Oct 15, 2019
amahussein pushed a commit to amahussein/hadoop that referenced this pull request Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants