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/08/11 18:42:53 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #2875: Add log messages to initialization, checkstyle fixes
EdColeman opened a new pull request, #2875:
URL: https://github.com/apache/accumulo/pull/2875
Logs when instance id is written to hdfs
- Add checking for successful file instance id creation
- Other checkstyle fixes
Supplements changes made in https://github.com/apache/accumulo/pull/2873
--
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] milleruntime commented on pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#issuecomment-1225781271
> @milleruntime - are you okay with this as is, or do you think there should be additional changes?
Just some javadoc clean up, then it can be merged.
--
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 #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#discussion_r944410083
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -257,10 +256,20 @@ private static boolean createDirs(VolumeManager fs, InstanceId instanceId, Set<S
fs.mkdirs(
new Path(new Path(baseDir, Constants.VERSION_DIR), "" + AccumuloDataVersion.get()),
new FsPermission("700"));
+ log.info("Created directory {}", baseDir);
Review Comment:
I don't see any javadoc in HDFS for why the underlying `FileSystem.mkdirs` call would return `false`. The result of the `fs.mkdirs` call above and below is unchecked. We may want to do something like:
```
boolean success = fs.mkdirs ...
if (!success && !fs.exists(Path)) {
throw new Something;
} else {
log.info("Created directory {}", baseDir);
}
```
Could use the same pattern for the iidPath file below.
--
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] milleruntime commented on a diff in pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#discussion_r943870711
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -257,10 +256,20 @@ private static boolean createDirs(VolumeManager fs, InstanceId instanceId, Set<S
fs.mkdirs(
new Path(new Path(baseDir, Constants.VERSION_DIR), "" + AccumuloDataVersion.get()),
new FsPermission("700"));
+ log.info("Created directory {}", baseDir);
+
Path iidLocation = new Path(baseDir, Constants.INSTANCE_ID_DIR);
fs.mkdirs(iidLocation);
- fs.createNewFile(new Path(iidLocation, instanceId.canonical()));
- log.info("Created directory {}", baseDir);
+ log.info("Created directory {}", iidLocation);
+
+ Path iidPath = new Path(iidLocation, instanceId.canonical());
+ boolean createSuccess = fs.createNewFile(iidPath);
+ if (!createSuccess || fs.exists(iidPath)) {
Review Comment:
Do we want to throw an error if the iidPath already exists? It could be verified somewhere else in `Initialize`
--
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] milleruntime commented on a diff in pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#discussion_r943868943
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -257,10 +256,20 @@ private static boolean createDirs(VolumeManager fs, InstanceId instanceId, Set<S
fs.mkdirs(
new Path(new Path(baseDir, Constants.VERSION_DIR), "" + AccumuloDataVersion.get()),
new FsPermission("700"));
+ log.info("Created directory {}", baseDir);
+
Path iidLocation = new Path(baseDir, Constants.INSTANCE_ID_DIR);
fs.mkdirs(iidLocation);
- fs.createNewFile(new Path(iidLocation, instanceId.canonical()));
- log.info("Created directory {}", baseDir);
+ log.info("Created directory {}", iidLocation);
+
+ Path iidPath = new Path(iidLocation, instanceId.canonical());
+ boolean createSuccess = fs.createNewFile(iidPath);
+ if (!createSuccess || fs.exists(iidPath)) {
Review Comment:
This could be misleading if the file already exists. It looks like the only way it will fail without error is if the file already exists. So I think you could just do:
```suggestion
if (fs.createNewFile(iidPath)) {
```
--
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] EdColeman commented on a diff in pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#discussion_r946838167
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -257,10 +256,20 @@ private static boolean createDirs(VolumeManager fs, InstanceId instanceId, Set<S
fs.mkdirs(
new Path(new Path(baseDir, Constants.VERSION_DIR), "" + AccumuloDataVersion.get()),
new FsPermission("700"));
+ log.info("Created directory {}", baseDir);
Review Comment:
Addressed in 152388d51b
--
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] EdColeman commented on a diff in pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#discussion_r944056939
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -257,10 +256,20 @@ private static boolean createDirs(VolumeManager fs, InstanceId instanceId, Set<S
fs.mkdirs(
new Path(new Path(baseDir, Constants.VERSION_DIR), "" + AccumuloDataVersion.get()),
new FsPermission("700"));
+ log.info("Created directory {}", baseDir);
+
Path iidLocation = new Path(baseDir, Constants.INSTANCE_ID_DIR);
fs.mkdirs(iidLocation);
- fs.createNewFile(new Path(iidLocation, instanceId.canonical()));
- log.info("Created directory {}", baseDir);
+ log.info("Created directory {}", iidLocation);
+
+ Path iidPath = new Path(iidLocation, instanceId.canonical());
+ boolean createSuccess = fs.createNewFile(iidPath);
+ if (!createSuccess || fs.exists(iidPath)) {
Review Comment:
I'm not sure want happens if it exists - didn't think about that. The check was based on the following assumptions
- createNewFile could return false on any hdfs error (forgot about exists) so we should check.
- the `fs.exists()` was specifically added to read that yea, verily the file was really written as hdfs said.
If I could have found a sync method, I would have added that too. Not really worried about performance of something that runs once per instance - and really trying to nail down if we have a timing issue or a concurrent test issue with the mini tests that claim they can find the instance.
--
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] EdColeman commented on a diff in pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#discussion_r946932278
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -251,16 +251,55 @@ private static boolean zookeeperAvailable(ZooReaderWriter zoo) {
}
}
+ /**
+ * Create the version directory and the instance id path and file. The method tries to create the
+ * directories and instance id file for all base directories provided unless an IOException is
+ * thrown. The IOException is not rethrown, but the method not try to create additional entries
+ * and will return false.
+ *
+ * @return false if an IOException occurred, true otherwise.
+ */
private static boolean createDirs(VolumeManager fs, InstanceId instanceId, Set<String> baseDirs) {
+ boolean success;
+
try {
for (String baseDir : baseDirs) {
- fs.mkdirs(
- new Path(new Path(baseDir, Constants.VERSION_DIR), "" + AccumuloDataVersion.get()),
- new FsPermission("700"));
+ log.debug("creating instance directories for base: {}", baseDir);
+
+ Path verDir =
+ new Path(new Path(baseDir, Constants.VERSION_DIR), "" + AccumuloDataVersion.get());
+ FsPermission permission = new FsPermission("700");
+
+ if (fs.exists(verDir)) {
+ FileStatus fsStat = fs.getFileStatus(verDir);
+ log.info("directory {} exists. Permissions match: {}", fsStat.getPath(),
+ fsStat.getPermission().equals(permission));
+ } else {
+ success = fs.mkdirs(verDir, permission);
+ log.info("Directory {} created - call returned {}", verDir, success);
+ }
+
Path iidLocation = new Path(baseDir, Constants.INSTANCE_ID_DIR);
- fs.mkdirs(iidLocation);
- fs.createNewFile(new Path(iidLocation, instanceId.canonical()));
- log.info("Created directory {}", baseDir);
+ if (fs.exists(iidLocation)) {
+ log.info("directory {} exists.", iidLocation);
+ } else {
+ success = fs.mkdirs(iidLocation);
+ log.info("Directory {} created - call returned {}", iidLocation, success);
+ }
+
+ Path iidPath = new Path(iidLocation, instanceId.canonical());
+
+ if (fs.exists(iidPath)) {
+ log.info("InstanceID file {} exists.", iidPath);
+ } else {
+ success = fs.createNewFile(iidPath);
+ // the exists() call provides positive check that the instanceId file is present
+ if (!success || fs.exists(iidPath)) {
+ log.info("Created instanceId file {} in hdfs", iidPath);
Review Comment:
The logic is actually incorrect - updated it with a12c71bd27. This PR is focused on providing log messages that we can use to debug the 'failed to find instance id' - looking at it in a broader scope, returning false instead of throwing the IOException seems like it could have been streamlined. But, I just thought we could benefit from some additional logging and the logic could be reworked separately.
--
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] EdColeman commented on pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#issuecomment-1225755468
@milleruntime - are you okay with this as is, or do you think there should be additional 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] milleruntime commented on a diff in pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#discussion_r953851136
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -367,7 +406,7 @@ private byte[] getRootPassword(InitialConfiguration initConfig, Opts opts, Strin
/**
* Create warning message related to initial password, if appropriate.
- *
+ * </p>
Review Comment:
```suggestion
```
--
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] milleruntime commented on a diff in pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#discussion_r946906841
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -251,16 +251,55 @@ private static boolean zookeeperAvailable(ZooReaderWriter zoo) {
}
}
+ /**
+ * Create the version directory and the instance id path and file. The method tries to create the
+ * directories and instance id file for all base directories provided unless an IOException is
+ * thrown. The IOException is not rethrown, but the method not try to create additional entries
+ * and will return false.
+ *
+ * @return false if an IOException occurred, true otherwise.
+ */
private static boolean createDirs(VolumeManager fs, InstanceId instanceId, Set<String> baseDirs) {
+ boolean success;
+
try {
for (String baseDir : baseDirs) {
- fs.mkdirs(
- new Path(new Path(baseDir, Constants.VERSION_DIR), "" + AccumuloDataVersion.get()),
- new FsPermission("700"));
+ log.debug("creating instance directories for base: {}", baseDir);
+
+ Path verDir =
+ new Path(new Path(baseDir, Constants.VERSION_DIR), "" + AccumuloDataVersion.get());
+ FsPermission permission = new FsPermission("700");
+
+ if (fs.exists(verDir)) {
+ FileStatus fsStat = fs.getFileStatus(verDir);
+ log.info("directory {} exists. Permissions match: {}", fsStat.getPath(),
+ fsStat.getPermission().equals(permission));
+ } else {
+ success = fs.mkdirs(verDir, permission);
+ log.info("Directory {} created - call returned {}", verDir, success);
+ }
+
Path iidLocation = new Path(baseDir, Constants.INSTANCE_ID_DIR);
- fs.mkdirs(iidLocation);
- fs.createNewFile(new Path(iidLocation, instanceId.canonical()));
- log.info("Created directory {}", baseDir);
+ if (fs.exists(iidLocation)) {
+ log.info("directory {} exists.", iidLocation);
+ } else {
+ success = fs.mkdirs(iidLocation);
+ log.info("Directory {} created - call returned {}", iidLocation, success);
+ }
+
+ Path iidPath = new Path(iidLocation, instanceId.canonical());
+
+ if (fs.exists(iidPath)) {
+ log.info("InstanceID file {} exists.", iidPath);
+ } else {
+ success = fs.createNewFile(iidPath);
+ // the exists() call provides positive check that the instanceId file is present
+ if (!success || fs.exists(iidPath)) {
+ log.info("Created instanceId file {} in hdfs", iidPath);
Review Comment:
Assuming `createNewFile()` can return false without throwing an error, I think there is still a problem with this logic. For example, if the `iidPath` doesn't exist but `fs.createNewFile(iidPath)` returns false, the `!success` will cause the if to short circuit and this message will print a misleading message.
Since we aren't quite sure of the behavior of these method calls, you could pull this chunk of logic out into a separate static method that we could be tested in a unit test.
--
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] EdColeman commented on a diff in pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#discussion_r946837735
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -257,10 +256,20 @@ private static boolean createDirs(VolumeManager fs, InstanceId instanceId, Set<S
fs.mkdirs(
new Path(new Path(baseDir, Constants.VERSION_DIR), "" + AccumuloDataVersion.get()),
new FsPermission("700"));
+ log.info("Created directory {}", baseDir);
+
Path iidLocation = new Path(baseDir, Constants.INSTANCE_ID_DIR);
fs.mkdirs(iidLocation);
- fs.createNewFile(new Path(iidLocation, instanceId.canonical()));
- log.info("Created directory {}", baseDir);
+ log.info("Created directory {}", iidLocation);
+
+ Path iidPath = new Path(iidLocation, instanceId.canonical());
+ boolean createSuccess = fs.createNewFile(iidPath);
+ if (!createSuccess || fs.exists(iidPath)) {
Review Comment:
Addressed in 152388d51b
--
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] Manno15 commented on a diff in pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
Manno15 commented on code in PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#discussion_r950734010
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -367,7 +406,7 @@ private byte[] getRootPassword(InitialConfiguration initConfig, Opts opts, Strin
/**
* Create warning message related to initial password, if appropriate.
- *
+ * </p>
Review Comment:
Not sure what this `</p>` is related to.
--
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] milleruntime commented on a diff in pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#discussion_r953853151
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -367,7 +406,7 @@ private byte[] getRootPassword(InitialConfiguration initConfig, Opts opts, Strin
/**
* Create warning message related to initial password, if appropriate.
- *
+ * </p>
Review Comment:
```suggestion
* <p>
```
--
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] EdColeman merged pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
EdColeman merged PR #2875:
URL: https://github.com/apache/accumulo/pull/2875
--
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] milleruntime commented on a diff in pull request #2875: Add log messages to initialization, checkstyle fixes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2875:
URL: https://github.com/apache/accumulo/pull/2875#discussion_r953850560
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -251,16 +251,55 @@ private static boolean zookeeperAvailable(ZooReaderWriter zoo) {
}
}
+ /**
+ * Create the version directory and the instance id path and file. The method tries to create the
+ * directories and instance id file for all base directories provided unless an IOException is
+ * thrown. The IOException is not rethrown, but the method not try to create additional entries
+ * and will return false.
+ *
Review Comment:
```suggestion
* thrown. If an IOException occurs, this method won't retry and will return false.
*
```
--
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