----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11861/#review21907 -----------------------------------------------------------
api/src/com/cloud/storage/template/TemplateInfo.java <https://reviews.apache.org/r/11861/#comment45197> .getName() is unnecessary. Just pass the class: private static Logger s_logger = Logger.getLogger(TemplateInfo.class); api/src/com/cloud/storage/template/TemplateInfo.java <https://reviews.apache.org/r/11861/#comment45198> Null is not a sufficient check for blank. Use StringUtils#isBlank to catch both null and instances containing only spaces. api/src/com/cloud/storage/template/TemplateInfo.java <https://reviews.apache.org/r/11861/#comment45199> WARN should be used to let an operator know there is a condition in the system that could lead to instability. A data cleanup will not result in eventual system instability. I recommend converting to debug or trace. api/src/com/cloud/storage/template/TemplateInfo.java <https://reviews.apache.org/r/11861/#comment45200> Why are fixing the path in both the getter and setter? I recommend fixing it in either method, not both. core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java <https://reviews.apache.org/r/11861/#comment45201> See previous comment regarding logger initialization. core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java <https://reviews.apache.org/r/11861/#comment45202> Extract this method to com.cloud.utils.FileUtil and reference from both CreateEntityDownloadURLCommand and TemplateInfo. Also, add unit tests for it. core/src/com/cloud/storage/template/TemplateLocation.java <https://reviews.apache.org/r/11861/#comment45203> Why are is the path being manipulated in addition to the work being down by the TemplateInfo class? These rules should be completely encapsulated in the TemplateInfo class. engine/schema/src/com/cloud/storage/VMTemplateHostVO.java <https://reviews.apache.org/r/11861/#comment45204> See previous note about logger initialization engine/schema/src/com/cloud/storage/VMTemplateHostVO.java <https://reviews.apache.org/r/11861/#comment45205> See previous note about extraction of this method and unit testing. server/src/com/cloud/storage/download/DownloadMonitorImpl.java <https://reviews.apache.org/r/11861/#comment45207> Wrap in an if (s_logger.isDebugEnabled()) block to avoid expensive string concatenation when DEBUG logging is not enabled. server/src/com/cloud/storage/download/DownloadMonitorImpl.java <https://reviews.apache.org/r/11861/#comment45206> Wrap in an if (s_logger.isDebugEnabled()) block to avoid expensive string concatenation when DEBUG logging is not enabled. server/src/com/cloud/template/TemplateManagerImpl.java <https://reviews.apache.org/r/11861/#comment45208> Wrap in an if (s_logger.isDebugEnabled()) block to avoid expensive string concatenation when DEBUG logging is not enabled. - John Burwell On June 14, 2013, 2:18 p.m., daan Hoogland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11861/ > ----------------------------------------------------------- > > (Updated June 14, 2013, 2:18 p.m.) > > > Review request for cloudstack. > > > Description > ------- > > double slash breaks windows based nfs servers [CLOUDSTACK-2968] > > > This addresses bug CLOUDSTACK-2968. > > > Diffs > ----- > > api/src/com/cloud/storage/template/TemplateInfo.java 6559d73 > core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java > 98a957f > core/src/com/cloud/agent/api/storage/DownloadAnswer.java bb7b8a9 > core/src/com/cloud/storage/template/TemplateLocation.java 58d023a > engine/schema/src/com/cloud/storage/VMTemplateHostVO.java b8dfc41 > > engine/storage/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java > a6880c3 > server/src/com/cloud/storage/download/DownloadListener.java 1d48803 > server/src/com/cloud/storage/download/DownloadMonitorImpl.java f72a563 > server/src/com/cloud/template/HypervisorTemplateAdapter.java 322f32e > server/src/com/cloud/template/TemplateManagerImpl.java 517d4ba > > Diff: https://reviews.apache.org/r/11861/diff/ > > > Testing > ------- > > database analysis > > > Thanks, > > daan Hoogland > >
