[
https://issues.apache.org/jira/browse/AMQ-7394?focusedWorklogId=396836&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-396836
]
ASF GitHub Bot logged work on AMQ-7394:
---------------------------------------
Author: ASF GitHub Bot
Created on: 03/Mar/20 16:13
Start Date: 03/Mar/20 16:13
Worklog Time Spent: 10m
Work Description: jbonofre commented on pull request #494: [AMQ-7394]
Simple first fix to use listener.hasSpace() when recovering message from JDBC
message store
URL: https://github.com/apache/activemq/pull/494
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 396836)
Time Spent: 20m (was: 10m)
> 'recovery' of messages does not take into account the PolicyMap/PolicyEntry
> 'memoryLimit', but takes 200 messages from the store
> --------------------------------------------------------------------------------------------------------------------------------
>
> Key: AMQ-7394
> URL: https://issues.apache.org/jira/browse/AMQ-7394
> Project: ActiveMQ
> Issue Type: Bug
> Components: JDBC
> Affects Versions: 5.15.2
> Reporter: Reinald Verheij
> Assignee: Jean-Baptiste Onofré
> Priority: Major
> Fix For: 5.16.0, 5.15.12
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> I believe this affects versions up to 5.15.11 too, since I don't see changes
> in the related code.
> Perhaps this issue is similar to the KahaDB JIRA AMQ-7126 which was fixed in
> 5.15.9?
> At Infor we use activeMQ 5.15.2 currently with the JDBC Persistence Adapter.
> When a queue (the tables in the DB for the queue) has many messages of a
> significant size (e.g. 200 messages of 5 MB) and I start the brokerService
> then it will 'recover' the messages. The first call will get 400 messages and
> when consuming and removing those subsequent calls will get 200 messages each
> *without* taking into account the configured memory limits per-broker or
> per-queue. So memory consumption is rather unconstrained here.
> I believe this is because [JDBCMessageStore.java
> recoverNextMessages|https://github.com/apache/activemq/blob/master/activemq-jdbc-store/src/main/java/org/apache/activemq/store/jdbc/JDBCMessageStore.java]
> always returns 'true' (at line 368 currently) without asking
> {{listener.hasSpace()}} (or rather the new {{canRecoveryNextMessage()}})
> {code:java} @Override
> public boolean recoverMessage(long sequenceId, byte[] data)
> throws Exception {
> Message msg = (Message)wireFormat.unmarshal(new
> ByteSequence(data));
> msg.getMessageId().setBrokerSequenceId(sequenceId);
>
> msg.getMessageId().setFutureOrSequenceLong(sequenceId);
> msg.getMessageId().setEntryLocator(sequenceId);
> listener.recoverMessage(msg);
> trackLastRecovered(sequenceId, msg.getPriority());
> return true;
> }{code}
> ... so the resultset iterator in DefaultJDBCAdapter::doRecoverNextMessages
> never jumps into the else branch to abort loading/recovering messages (at
> line 1093 currently) {code:java} while (rs.next() && count <
> maxReturned) {
> if (listener.recoverMessage(rs.getLong(1),
> getBinaryData(rs, 2))) {
> count++;
> } else {
> LOG.debug("Stopped recover next messages");
> break;
> }
> }{code}
> I did a workaround by subclassing the TransactJDBCAdapter and then wrapping
> the listener to impose my own limit (however this is a separately configured
> limit; it is not coupled with the memory limits of the broker or the queue,
> so it is more a workaround than a solution; but it avoids customizing
> activeMQ code, the workaround is in our own product code.
> My suggested improvement is:
> * build the listener.hasSpace() (or rather the new
> {{canRecoveryNextMessage()}}) as was done for KahaDB in AMQ-7126
> * AND do statement.cancel() if the listener does not have the space, to avoid
> performance trouble, see below.
> h3. Explanation that leads to the suggestion to do statement.cancel() if
> listener.hasSpace() (or rather the new {{canRecoveryNextMessage()}}) returns
> false.
> When the recover() produced fewer than requested messages due to size
> restrictions (e.g. it produced 10 messages instead of 200 because they were
> large) I noticed delays from consumer perspective of 4 seconds (with local
> sql server on ssd, so must be higher with clustered SQL server...). It
> appeared to be taking a long time to close the resultset. The answer is that
> the resultset needs to be exhausted during rs.close() because otherwise the
> connection can't be reused later.
> Source: someone asked this on the msft forum in 2009: {quote}I'm encountering
> a strange issue with the ResultSet.close() command with the new 2.0 jdbc
> drivers (not the JDBC 4.0 version).
> If I run a query that returns a large result set and just iterate through the
> first couple of rows and then attempt to call close() on the result set the
> close() call hangs for quite some time insted of closing almost instantly as
> past versions of the driver have done with this same code. {quote} and then
> David Olix responded saying that it's expected behavior in SQL Server driver
> as of 2.0, and suggested to cancel the sql statement. {quote}Hi,
> Thank you for considering the SQL Server JDBC Driver 2.0. And thank you for
> your post. It illustrates a very interesting point about data retrieval with
> the 2.0 driver...
> Short answer:
> The behavior you are seeing is expected in the default configuration of the
> 2.0 driver. You should be able to revert to the previous behavior by setting
> the connection property "responseBuffering=full". But read on for why that
> may not be your best solution...
> Long answer:
> When SQL Server returns a large number of rows from a SELECT query, the JDBC
> driver must process all of them to be able to execute the next query on the
> same connection. Retrieving those rows from the server can take a
> substantial amount of time, especially over a slower network connection. By
> default, older versions of the driver retrieved all of the rows into memory
> when the statement was executed. The 1.2 driver introduced a new optional
> behavior called adaptive response buffering, which allows the driver to
> retrieve row data (and other results) from the server as the application
> accesses/demands it. The benefit of adaptive buffering is lower client side
> memory usage -- much lower for large result sets -- with amortized latency
> across ResultSet accessor methods for apps that process all the result data
> in the order returned. The 2.0 driver uses adaptive response buffering by
> default. In your case, the app accesses only the first few rows. So
> ResultSet.close() takes longer because it must read and discard the rest of
> the rows.
> That raises the question:
> If your application is only interested in the first few rows, why does your
> query return thousands of them?
> Performance and client-side memory usage would be much improved if you can
> structure the query to return only the potentially interesting rows. Put
> another way, it's best not to query for data that you're not going to use.
> If that is not an option, and you have no other choice but to query for all
> of the rows, there are other options that may yield improved performance
> still:
> Consider cancelling the statement before closing the ResultSet. Cancelling
> tells the driver to notify the server that it is not interested in any
> execution results (including remaining rows) that haven't already been
> received. You have to weigh the cost of cancellation, which incurs a short
> round trip request/response to the server, against the cost of receiving the
> rest of the rows when using this technique, but it sounds like it would be a
> definite win if there are tens of thousands of rows left.
> Or consider using a server-side cursor when executing the query. You can do
> this by specifying
> com.microsoft.sqlserver.jdbc.SQLServerResultSet.TYPE_SS_SERVER_CURSOR_FORWARD_ONLY
> rather than java.sql.ResultSet.TYPE_FORWARD_ONLY for the scrollability
> parameter to the createStatement()/prepareStatement() call. Server-side
> cursors allow the driver to retrieve rows a few at a time. There's a cost
> with server cursors too: a round trip to fetch each block of rows. But
> closing the ResultSet closes the server cursor at which point no further rows
> are returned. But before using a server cursor, make sure the query is
> cursorable, or SQL Server will just return all the rows anyway...
> The last option is the one from the short answer: use full response buffering
> rather than adaptive response buffering. This may be the least desirable
> solution, however, since the app still has to wait for all the rows to be
> received from the server (in executeQuery() though, rather than
> ResultSet.close()). But rather than discarding the uninteresting rows, it
> has to buffer them, which could lead to an OutOfMemoryError if the result set
> is large enough.
> HTH,
> --David Olix [SQL Server]{quote}
> Source (not sure if URL is stable, hence the full quote):
> https://social.msdn.microsoft.com/Forums/security/en-US/7ceee9a8-09d8-4fbd-9918-0ca065fa182e/resultsetclose-hangs-with-new-jdbc-20?forum=sqldataaccess
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
