You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by DaanHoogland <gi...@git.apache.org> on 2015/08/04 00:52:51 UTC
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
GitHub user DaanHoogland opened a pull request:
https://github.com/apache/cloudstack/pull/654
Cloudstack 8656: do away with more silently ignoring exceptions
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaanHoogland/cloudstack CLOUDSTACK-8656
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/654.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #654
----
commit 6e0813159f19ed2f658d8153a099e82d91e6add8
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-08-03T22:18:26Z
CLOUDSTACK-8656: log initialisation logging
commit cc74f806d8410fa137369400db2b021651f07b55
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-08-03T22:41:10Z
CLOUDSTACK-8656: closeable in vmsd reader
moved closeable util function up the hierarchy
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: [WIP] Cloudstack 8656: do away with more ...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-128595555
Aside from my "break" question and indentation request, LGTM.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: [WIP] Cloudstack 8656: do away with more ...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36490966
--- Diff: utils/src/com/cloud/utils/nio/NioConnection.java ---
@@ -334,6 +333,7 @@ protected void processTodos() {
try {
((SocketChannel)(todo.key)).close();
} catch (IOException ignore) {
+ s_logger.info("[ignored] socket channel");
--- End diff --
Maybe indent here.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/654
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: [WIP] Cloudstack 8656: do away with more ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36173727
--- Diff: utils/src/com/cloud/utils/AutoCloseableUtil.java ---
@@ -0,0 +1,20 @@
+package com.cloud.utils;
--- End diff --
thanks will add, @koushik-das
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131212079
@DaanHoogland I agree, we should only squash if there is no value to have the commits separated. I usually ask for squashing when feedback was processed in a separate commit (say remove a comment or fix a small thing). That you want squashed so it becomes an atomic commit. If commits are already atomic, I'm OK with not squashing. It's your call here ;-)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: [WIP] Cloudstack 8656: do away with more ...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36490932
--- Diff: services/console-proxy/server/src/com/cloud/consoleproxy/vnc/packet/server/RawRect.java ---
@@ -50,26 +52,27 @@ public void paint(BufferedImage image, Graphics2D graphics) {
switch (dataBuf.getDataType()) {
- case DataBuffer.TYPE_INT: {
- // We chose RGB888 model, so Raster will use DataBufferInt type
- DataBufferInt dataBuffer = (DataBufferInt)dataBuf;
+ case DataBuffer.TYPE_INT: {
+ // We chose RGB888 model, so Raster will use DataBufferInt type
+ DataBufferInt dataBuffer = (DataBufferInt)dataBuf;
- int imageWidth = image.getWidth();
- int imageHeight = image.getHeight();
+ int imageWidth = image.getWidth();
+ int imageHeight = image.getHeight();
- // Paint rectangle directly on buffer, line by line
- int[] imageBuffer = dataBuffer.getData();
- for (int srcLine = 0, dstLine = y; srcLine < height && dstLine < imageHeight; srcLine++, dstLine++) {
- try {
- System.arraycopy(buf, srcLine * width, imageBuffer, x + dstLine * imageWidth, width);
- } catch (IndexOutOfBoundsException e) {
- }
+ // Paint rectangle directly on buffer, line by line
+ int[] imageBuffer = dataBuffer.getData();
+ for (int srcLine = 0, dstLine = y; srcLine < height && dstLine < imageHeight; srcLine++, dstLine++) {
+ try {
+ System.arraycopy(buf, srcLine * width, imageBuffer, x + dstLine * imageWidth, width);
+ } catch (IndexOutOfBoundsException e) {
+ s_logger.info("[ignored] buffer overflow!?!", e);
}
- break;
}
+ break;
--- End diff --
It looks like this change of location for "break" fixes a bug (versus being "cosmetic"). Is that true? Prior to the fix, it appears the loop could only be executed at most one time.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36166324
--- Diff: utils/src/com/cloud/utils/AutoCloseableUtil.java ---
@@ -0,0 +1,20 @@
+package com.cloud.utils;
--- End diff --
License?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36774764
--- Diff: services/console-proxy/server/src/com/cloud/consoleproxy/vnc/packet/server/RawRect.java ---
@@ -50,26 +52,27 @@ public void paint(BufferedImage image, Graphics2D graphics) {
switch (dataBuf.getDataType()) {
- case DataBuffer.TYPE_INT: {
- // We chose RGB888 model, so Raster will use DataBufferInt type
- DataBufferInt dataBuffer = (DataBufferInt)dataBuf;
+ case DataBuffer.TYPE_INT: {
+ // We chose RGB888 model, so Raster will use DataBufferInt type
+ DataBufferInt dataBuffer = (DataBufferInt)dataBuf;
- int imageWidth = image.getWidth();
- int imageHeight = image.getHeight();
+ int imageWidth = image.getWidth();
+ int imageHeight = image.getHeight();
- // Paint rectangle directly on buffer, line by line
- int[] imageBuffer = dataBuffer.getData();
- for (int srcLine = 0, dstLine = y; srcLine < height && dstLine < imageHeight; srcLine++, dstLine++) {
- try {
- System.arraycopy(buf, srcLine * width, imageBuffer, x + dstLine * imageWidth, width);
- } catch (IndexOutOfBoundsException e) {
- }
+ // Paint rectangle directly on buffer, line by line
+ int[] imageBuffer = dataBuffer.getData();
+ for (int srcLine = 0, dstLine = y; srcLine < height && dstLine < imageHeight; srcLine++, dstLine++) {
+ try {
+ System.arraycopy(buf, srcLine * width, imageBuffer, x + dstLine * imageWidth, width);
+ } catch (IndexOutOfBoundsException e) {
+ s_logger.info("[ignored] buffer overflow!?!", e);
}
- break;
}
+ break;
--- End diff --
I guess it's OK. It looks like it would fix a bug (the for loop would only execute at most once before).
If you're OK with it, I'm OK with it.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131181130
You know I am a great advocate of not squashing, right. All commits are atomic. Yes there is a lot because the work has also been scattered and a lot.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131214770
@bhaisaab Indeed, let's fix forward when needed :-) There's always a risk, but I'm happy to see this work being done :+1:
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-129990097
LGTM
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131175032
look alright, but could it introduce any regressions in the upgrade path? Also, I would love to see a single squashed patched with the changes given ~30 patches for ~+400/~-800 change
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131178239
All kinds of regressions may be introduced. This is (one of the reasons) why I will not squash. for a single regression we only have to revert a single commit, for the bunch we will have to revert the merge commit.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36167090
--- Diff: vmware-base/src/com/cloud/hypervisor/vmware/mo/SnapshotDescriptor.java ---
@@ -166,12 +168,7 @@ public void removeDiskReferenceFromSnapshot(String diskFileName) {
assert (false);
s_logger.error("Unexpected exception ", e);
} finally {
- if (out != null) {
- try {
- out.close();
- } catch (IOException e) {
- }
- }
+ closeAutoCloseable(out, "buffered write for bytestream");
--- End diff --
Yes, due to the amount of tedious work I am choosing try-with-resource only when it is easy. It is in this case so I will change it, thanks @koushik-das
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: [WIP] Cloudstack 8656: do away with more ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36763863
--- Diff: services/console-proxy/server/src/com/cloud/consoleproxy/vnc/packet/server/RawRect.java ---
@@ -50,26 +52,27 @@ public void paint(BufferedImage image, Graphics2D graphics) {
switch (dataBuf.getDataType()) {
- case DataBuffer.TYPE_INT: {
- // We chose RGB888 model, so Raster will use DataBufferInt type
- DataBufferInt dataBuffer = (DataBufferInt)dataBuf;
+ case DataBuffer.TYPE_INT: {
+ // We chose RGB888 model, so Raster will use DataBufferInt type
+ DataBufferInt dataBuffer = (DataBufferInt)dataBuf;
- int imageWidth = image.getWidth();
- int imageHeight = image.getHeight();
+ int imageWidth = image.getWidth();
+ int imageHeight = image.getHeight();
- // Paint rectangle directly on buffer, line by line
- int[] imageBuffer = dataBuffer.getData();
- for (int srcLine = 0, dstLine = y; srcLine < height && dstLine < imageHeight; srcLine++, dstLine++) {
- try {
- System.arraycopy(buf, srcLine * width, imageBuffer, x + dstLine * imageWidth, width);
- } catch (IndexOutOfBoundsException e) {
- }
+ // Paint rectangle directly on buffer, line by line
+ int[] imageBuffer = dataBuffer.getData();
+ for (int srcLine = 0, dstLine = y; srcLine < height && dstLine < imageHeight; srcLine++, dstLine++) {
+ try {
+ System.arraycopy(buf, srcLine * width, imageBuffer, x + dstLine * imageWidth, width);
+ } catch (IndexOutOfBoundsException e) {
+ s_logger.info("[ignored] buffer overflow!?!", e);
}
- break;
}
+ break;
--- End diff --
So it seemed to me @mike-tutkowski . Is this a worry concerning this PR?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131164822
LGTM
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36166742
--- Diff: vmware-base/src/com/cloud/hypervisor/vmware/mo/SnapshotDescriptor.java ---
@@ -166,12 +168,7 @@ public void removeDiskReferenceFromSnapshot(String diskFileName) {
assert (false);
s_logger.error("Unexpected exception ", e);
} finally {
- if (out != null) {
- try {
- out.close();
- } catch (IOException e) {
- }
- }
+ closeAutoCloseable(out, "buffered write for bytestream");
--- End diff --
Why not use the try-with-resource syntax instead of doing all this? IIRC you had submitted earlier commits with similar changes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36141156
--- Diff: framework/db/src/com/cloud/utils/db/DbUtil.java ---
@@ -43,8 +43,10 @@
import org.apache.log4j.Logger;
+import static com.cloud.utils.AutoCloseableUtil.closeAutoCloseable;
+
public class DbUtil {
- protected final static Logger s_logger = Logger.getLogger(DbUtil.class);
+ public final static Logger s_logger = Logger.getLogger(DbUtil.class);
--- End diff --
Just curious why we went from protected to public here?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131179496
Another way of fixing a regression could be to simply fix the issue, instead of reverting at all.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656: do away with more silent...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36141245
--- Diff: utils/src/com/cloud/utils/AutoCloseableUtil.java ---
@@ -0,0 +1,20 @@
+package com.cloud.utils;
+
+import org.apache.log4j.Logger;
+
+public class AutoCloseableUtil {
+ public final static Logger s_logger = Logger.getLogger(AutoCloseableUtil.class);
--- End diff --
Do we want this public or maybe private?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---