You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by gaohoward <gi...@git.apache.org> on 2018/09/12 03:15:29 UTC
[GitHub] activemq-artemis pull request #2305: NO-JIRA - Page.write() should throw exc...
GitHub user gaohoward opened a pull request:
https://github.com/apache/activemq-artemis/pull/2305
NO-JIRA - Page.write() should throw exception if file is closed
In Page.write(final PagedMessage message) if the page file is closed
it returns silently. The caller has no way to know that if the message
is paged to file or not. It should throw an exception so that the
caller can handle it correctly.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gaohoward/activemq-artemis a_page_write
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2305.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 #2305
----
commit 2bf2a3918a89c24f68de00a2dcc4630bf9b03409
Author: Howard Gao <ho...@...>
Date: 2018-09-12T03:11:02Z
NO-JIRA - Page.write() should throw exception if file is closed
In Page.write(final PagedMessage message) if the page file is closed
it returns silently. The caller has no way to know that if the message
is paged to file or not. It should throw an exception so that the
caller can handle it correctly.
----
---
[GitHub] activemq-artemis issue #2305: NO-JIRA - Page.write() should throw exception ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2305
@gaohoward I think this warrants a JIRA.
---
[GitHub] activemq-artemis issue #2305: NO-JIRA - Page.write() should throw exception ...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/2305
@jbertram Done. thx.
---
[GitHub] activemq-artemis issue #2305: NO-JIRA - Page.write() should throw exception ...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/2305
https://issues.apache.org/jira/browse/ARTEMIS-2088
---
[GitHub] activemq-artemis pull request #2305: NO-JIRA - Page.write() should throw exc...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2305#discussion_r217426008
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/Page.java ---
@@ -223,7 +223,7 @@ private int read(StorageManager storage, ActiveMQBuffer fileBuffer, List<PagedMe
public synchronized void write(final PagedMessage message) throws Exception {
if (!file.isOpen()) {
- return;
+ throw new IllegalStateException("can't write to closed file " + file);
--- End diff --
ok will do. thx.
---
[GitHub] activemq-artemis issue #2305: NO-JIRA - Page.write() should throw exception ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/2305
@gaohoward, looks like this needs to be rebased.
---
[GitHub] activemq-artemis issue #2305: NO-JIRA - Page.write() should throw exception ...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/2305
@clebertsuconic I've added more details to the commit comment.
---
[GitHub] activemq-artemis pull request #2305: NO-JIRA - Page.write() should throw exc...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2305#discussion_r217242750
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/Page.java ---
@@ -223,7 +223,7 @@ private int read(StorageManager storage, ActiveMQBuffer fileBuffer, List<PagedMe
public synchronized void write(final PagedMessage message) throws Exception {
if (!file.isOpen()) {
- return;
+ throw new IllegalStateException("can't write to closed file " + file);
--- End diff --
This is from https://github.com/hornetq/hornetq/pull/2122
(long forgotten by me. But as the pr noted that this causes random failure in PagingTest#testExpireLargeMessageOnPaging()).
---
[GitHub] activemq-artemis pull request #2305: NO-JIRA - Page.write() should throw exc...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2305#discussion_r217384203
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/Page.java ---
@@ -223,7 +223,7 @@ private int read(StorageManager storage, ActiveMQBuffer fileBuffer, List<PagedMe
public synchronized void write(final PagedMessage message) throws Exception {
if (!file.isOpen()) {
- return;
+ throw new IllegalStateException("can't write to closed file " + file);
--- End diff --
ok.. I think you should throw a proper exception, extending ActiveMQException:
ActiveMQIOErrorException would be perfect there.
---
[GitHub] activemq-artemis pull request #2305: NO-JIRA - Page.write() should throw exc...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2305#discussion_r217573949
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/Page.java ---
@@ -223,7 +224,7 @@ private int read(StorageManager storage, ActiveMQBuffer fileBuffer, List<PagedMe
public synchronized void write(final PagedMessage message) throws Exception {
if (!file.isOpen()) {
- return;
+ throw new ActiveMQIOErrorException("can't write to closed file " + file);
--- End diff --
Use a code on loggers?
---
[GitHub] activemq-artemis pull request #2305: NO-JIRA - Page.write() should throw exc...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2305#discussion_r217092142
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/Page.java ---
@@ -223,7 +223,7 @@ private int read(StorageManager storage, ActiveMQBuffer fileBuffer, List<PagedMe
public synchronized void write(final PagedMessage message) throws Exception {
if (!file.isOpen()) {
- return;
+ throw new IllegalStateException("can't write to closed file " + file);
--- End diff --
IllegalSTateException is not much better on this context.
Do you have a case where this is happening?
We shouldn't have a case where the file is open and still trying to write it.
Where do you see it?
---
[GitHub] activemq-artemis pull request #2305: NO-JIRA - Page.write() should throw exc...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2305#discussion_r217584046
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/Page.java ---
@@ -223,7 +224,7 @@ private int read(StorageManager storage, ActiveMQBuffer fileBuffer, List<PagedMe
public synchronized void write(final PagedMessage message) throws Exception {
if (!file.isOpen()) {
- return;
+ throw new ActiveMQIOErrorException("can't write to closed file " + file);
--- End diff --
ok. I thought about a jira before but I didn't do it as the code change is so few. :)
---