[
https://issues.apache.org/jira/browse/STORM-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15015250#comment-15015250
]
ASF GitHub Bot commented on STORM-876:
--------------------------------------
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`
> Dist Cache: Basic Functionality
> -------------------------------
>
> Key: STORM-876
> URL: https://issues.apache.org/jira/browse/STORM-876
> Project: Apache Storm
> Issue Type: Improvement
> Components: storm-core
> Reporter: Robert Joseph Evans
> Assignee: Robert Joseph Evans
> Attachments: DISTCACHE.md, DistributedCacheDesignDocument.pdf
>
>
> Basic functionality for the Dist Cache feature.
> As part of this a new API should be added to support uploading and
> downloading dist cache items. storm-core.ser, storm-conf.ser and storm.jar
> should be written into the blob store instead of residing locally. We need a
> default implementation of the blob store that does essentially what nimbus
> currently does and does not need anything extra. But having an HDFS backend
> too would be great for scalability and HA.
> The supervisor should provide a way to download and manage these blobs and
> provide a working directory for the worker process with symlinks to the
> blobs. It should also allow the blobs to be updated and switch the symlink
> atomically to point to the new blob once it is downloaded.
> All of this is already done by code internal to Yahoo! we are in the process
> of getting it ready to push back to open source shortly.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
