Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/845#issuecomment-158283901
Utils.java
* In general, the new methods we are adding here might be better off
somewhere in the blobstore package.
* Check indentation of changed code. I see some indented at 2 spaces, some
at 4. Also, I see some inconsistent initial indentation.
* Some of these methods are synchronized and hold a lock on Utils while
they access ZK. I can see this causing lots of problems. Can we try and use
more granular locking? Some of this can be helped by moving the code to a more
appropriate class or namespace.
```
447 nconf.put(Config.BLOBSTORE_CLEANUP_ENABLE, new Boolean(true));
```
I read it is dangerous to create instances of Boolean with given values,
and is preferable to use `Boolean.TRUE` and `Boolean.FALSE`.
```
452 // Meant to be called only by the supervisor for
stormjar/stormconf/stormcode files.
453 public static void downloadResourcesAsSupervisor(Map conf, String
key, String localFile,
454 ClientBlobStore cb)
throws AuthorizationExcep tion, KeyNotFoundException, IOException {
455 final int MAX_RETRY_ATTEMPTS = 2;
456 final int ATTEMPTS_INTERVAL_TIME = 100;
457 for (int retryAttempts = 0; retryAttempts < MAX_RETRY_ATTEMPTS;
retryAttempts++) {
458 if (downloadResourcesAsSupervisorAttempt(cb, key, localFile)) {
459 break;
460 }
461 Utils.sleep(ATTEMPTS_INTERVAL_TIME);
462 }
463 //NO Exception on error the supervisor will try again after a while
464 }
```
* `conf` parameter not used.
* The values here are hard-coded and are small. What benefit does the
sleep have? Could we unroll the `for` loop?
* If this is meant for the supervisor, does it belong in supervisor.clj?
```
486 } finally {
487 try {
488 if (out != null) out.close();
489 } catch (IOException ignored) {}
490 try {
491 if (in != null) in.close();
492 } catch (IOException ignored) {}
493 }
```
* minor: braces for the `if` statements
```
510 try {
511 ReadableBlobMeta metadata = cb.getBlobMeta(key);
512 nimbusBlobVersion = metadata.get_version();
513 } catch (AuthorizationException | KeyNotFoundException exp) {
514 throw exp;
515 } catch (TException e) {
516 throw new RuntimeException(e);
517 }
```
Can we ever catch a `TException` here?
```
562 // only works on operating systems that support posix
563 public static void restrictPermissions(String baseDir) {
564 try {
565 Set<PosixFilePermission> perms = new
HashSet<PosixFilePermission>(
566 Arrays.asList(PosixFilePermission.OWNER_READ,
PosixFilePermission.OWNER_WRITE,
567 PosixFilePermission.OWNER_EXECUTE,
PosixFilePermission.GROUP_READ,
568 PosixFilePermission.GROUP_EXECUTE));
569
Files.setPosixFilePermissions(FileSystems.getDefault().getPath(baseDir), perms);
570 } catch (IOException e) {
571 throw new RuntimeException(e);
572 }
573 }
```
If this is only used by the supervisor, should we move it to supervisor.clj?
```
627 public static byte[] thriftSerialize(TBase t) {
641 public static <T> T thriftDeserialize(Class c, byte[] b, int
offset, int length) {
652 public static <T> T thriftDeserialize(Class c, byte[] b) {
```
* `thriftDeserialize(Class c, byte[] b, int offset, int length)` does not
appear to be used anywhere
* The other two methods look nearly identical to code that is in Trident.
Could we make these common?
```
756 * @param in InputStrem to read from
```
`InputStream`
```
928 // java equivalent of util.on-windows?
929 public static boolean onWindows() {
930 return System.getenv("OS") == "Windows_NT";
931 }
```
* Use `.equals` for String
* Remove comment
* Remove `util.on-windows?` and have clojure code call into this one.
```
933 public static long unpack(File localrsrc, File dst) throws
IOException {
```
This always returns `0`, and the only place it's called ignores the return
value. We can make it `void`.
```
1102 * Takes an input dir or file and returns the du on that local
directory. Very basic
```
`du` -> `disk usage`
```
1375 if (isSuccess != true) {
```
`!isSuccess`
```
1405 public static synchronized void updateKeyForBlobStore (Map conf,
BlobStore blobStore, CuratorFramework zkClient, String key, NimbusInfo
nimbusDetails) {
1406 try {
1407 // This is to avoid mocking of unit tests at several places
as nimbusDetails
1408 // and moreover the nimbusInfo is not required for
HdfsBlobstore only for LocalFsBlobstore
1409 if (nimbusDetails == null) {
1410 return;
1411 }
1412 boolean isListContainsCurrentNimbusInfo = false;
1413 List<String> stateInfo = new ArrayList<String>();
```
* Check indentation throughout this method
* Remove extra space before signature in declaration
* If `nimbusDetails` is not required for HdfsBlobStore, why are we
returning immediately if it is `null`? Won't that break the HDFS case?
* Could this be a member method of a BlobStore instead of a static
method here in Utils?
* No need to initialize `stateInfo`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
