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 2022/11/14 17:09:46 UTC
[GitHub] [accumulo] dlmarion opened a new pull request, #3076: Call FSDataOutputStream.setDropBehind for WAL files
dlmarion opened a new pull request, #3076:
URL: https://github.com/apache/accumulo/pull/3076
See description of HDFS property dfs.datanode.drop.cache.behind.writes for a full explanation of what this does. This will tell the datanode to drop this file from the page cache when done writing.
--
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 #3076: Call FSDataOutputStream.setDropBehind for WAL files
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #3076:
URL: https://github.com/apache/accumulo/pull/3076#issuecomment-1314223461
This change is not in response to a new reported issue, but something that I have seen on busy clusters in the past. On busy clusters I have seen the case where the page cache flush process is very active. I think there is an opportunity here to help alleviate the cache pressure by telling the operating system that it doesn't need to cache the WAL after we write it, because we aren't going to be reading it. In fact, there is likely another commit here that I could make that is the opposite side of this - we can tell the operating system to drop or not cache the WAL during recovery as we are going to read it once and then be done with it.
IIRC, under the hood this calls [posix_fadvise](https://linux.die.net/man/2/posix_fadvise) and the native code is located [here](https://github.com/apache/hadoop/blame/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c#L167).
--
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 closed pull request #3076: Call FSDataOutputStream.setDropBehind for WAL files
Posted by GitBox <gi...@apache.org>.
dlmarion closed pull request #3076: Call FSDataOutputStream.setDropBehind for WAL files
URL: https://github.com/apache/accumulo/pull/3076
--
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 #3076: Call FSDataOutputStream.setDropBehind for WAL files
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3076:
URL: https://github.com/apache/accumulo/pull/3076#issuecomment-1314294641
Also, we don't need to label 3.0.0 as a target version, because unless 3.0.0 is released prior to 2.1.1, everything in a patch release is always included in the next `.0` release.
--
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 #3076: Call FSDataOutputStream.setDropBehind for WAL files
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #3076:
URL: https://github.com/apache/accumulo/pull/3076#issuecomment-1314306578
Closed in favor of #3077
--
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 #3076: Call FSDataOutputStream.setDropBehind for WAL files
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3076:
URL: https://github.com/apache/accumulo/pull/3076#issuecomment-1314308430
FWIW, if you just force push to your branch, you don't need to create a new 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 a diff in pull request #3076: Call FSDataOutputStream.setDropBehind for WAL files
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3076:
URL: https://github.com/apache/accumulo/pull/3076#discussion_r1021878316
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java:
##########
@@ -417,6 +417,13 @@ public synchronized void open(String address) throws IOException {
else
logFile = fs.create(logfilePath, true, 0, replication, blockSize);
+ // Tell the DataNode that the write ahead log does not need to be cached in the OS page cache
+ try {
+ logFile.setDropBehind(Boolean.TRUE);
Review Comment:
It's weird that the API parameter is a nullable `Boolean` and not the primitive `boolean`. I see that it is marked `@InterfaceStability.Evolving`, but it has been present since at least 3.0.0, so I think it's probably fine.
--
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 #3076: Call FSDataOutputStream.setDropBehind for WAL files
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #3076:
URL: https://github.com/apache/accumulo/pull/3076#issuecomment-1314274166
IIRC in previous situations like this, you had suggested to people that changes should be made in the most recent version and then cherry-picked backwards.
--
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 #3076: Call FSDataOutputStream.setDropBehind for WAL files
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3076:
URL: https://github.com/apache/accumulo/pull/3076#issuecomment-1314290954
> IIRC in previous situations like this, you had suggested to people that changes should be made in the most recent version and then cherry-picked backwards.
The circumstances differ here from that suggestion in a several important ways:
1. Both 2.x and 1.x versions had releases, and the proposed change was only being considered for the earlier 1.x version without consideration of the newer 2.x versions,
2. The code had already changed substantially in 2.x, which would have required a different design from the proposed 1.x change, meaning we couldn't just consider the change in 1.x; consideration of the change in 2.x may have informed design choices for the 1.x version (it would be bad if 1.x went one direction, and 2.x went a different direction, so we needed to consider 2.x in order to ensure the changes in 1.x were headed in the same direction),
3. The proposed changes then were non-trivial, and were still being polished, and it would have been premature to apply to an older stable release without fully understanding its impact, and
4. Depending on how things went after considering the changes in 2.x, we may have decided we didn't want to make the changes in 1.x
In this case, however:
1. there is no newer released version to consider,
2. the code between the branches is still identical,
3. the changes are trivial, and
4. we know up-front that we want to apply the changes to the earlier branch
Under circumstances like this, it's a matter of considering what is most efficient for tooling. If this were applied to the main branch, we'd cherry-pick the changes directly to the 2.1 branch, then merge forward back into the main branch, resulting in two identical commits in the history of the main branch. There's no reason to do that here, since the end result is the same. It's better under these circumstances to apply to the 2.1 branch and do one merge into the main branch, leaving only the one substantive commit and one trivial merge commit.
--
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 a diff in pull request #3076: Call FSDataOutputStream.setDropBehind for WAL files
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3076:
URL: https://github.com/apache/accumulo/pull/3076#discussion_r1021932231
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java:
##########
@@ -417,6 +417,13 @@ public synchronized void open(String address) throws IOException {
else
logFile = fs.create(logfilePath, true, 0, replication, blockSize);
+ // Tell the DataNode that the write ahead log does not need to be cached in the OS page cache
+ try {
+ logFile.setDropBehind(Boolean.TRUE);
Review Comment:
Javadoc says that if parameter is null, then it defaults to configuration, which should be the value of `dfs.datanode.drop.cache.behind.writes`
--
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 #3076: Call FSDataOutputStream.setDropBehind for WAL files
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #3076:
URL: https://github.com/apache/accumulo/pull/3076#issuecomment-1314272943
Failed test is due to connection timeout downloading Maven dependency
--
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