Uploaded image for project: 'Blazegraph (by SYSTAP)'
  1. Blazegraph (by SYSTAP)
  2. BLZG-84

Transaction identifiers assigned beyond the last commit time.

    Details

    • Type: Task
    • Status: Done
    • Resolution: Done
    • Affects Version/s: QUADS_QUERY_BRANCH
    • Fix Version/s: None
    • Component/s: Other

      Description

      An issue has been observed with the BigdataSail and the NanoSparqlServer where the latter obtains a read-only transaction from the last commit time on the database and the former treats that transaction identifier as a commit time and attempts to obtain a read-only transaction for that timestamp. This was causing an exception to be thrown indicating that the transaction identifier (when interpreted as a timestamp) was GT the lastCommitTime on the database.

      The goodness of obtaining a read-only transaction from a read-only transaction are questionable. This is probably not incorrect, but it could lead to nested acquisition of read-only transactions from read-only transactions, which does not add any value and will impose unnecessary latency on operations which are already protected by a read-lock.

      Read-write transaction identifers are readily identified by their sign bit, which is negative. However, currently, there is no way to distinguish timestamps from read-only transaction identifiers. This means that the code path must be recognized in advanced and appropriately distinguished methods called, e.g., getReadOnlyConnectionWithTimestamp(long) versus, getConnectionWithTx(long).

      Rather than develop such complicated naming conventions, it might be worth while to propagate a flyweight object to serve as a transaction identifier. This was differentiate read history operations without read-locks from those with read-locks based on the method signature. However, this again can cause us to have twice as method methods on the API.

        Activity

        Hide
        bryanthompson bryanthompson added a comment -

        Committed revision 3804 provides a workaround for the exception thrown out of the BigdataSail when a read-only transaction identifier obtained from the lastCommitTime is passed to BigdataSail#getReadOnlyConnection(long). This code path will currently create a second read-only transaction. While this introduces uncessary latency into operations which already have a read-only transaction, it does permit correct functioning of the NanoSparqlServer
        - albeit by defeating an optimization in which the NanoSparqlServer was using a single read-only transaction to answer concurrent queries. For these reasons, this workaround is not ideal. One could have equally modified NanoSparqlServer to not obtain a transaction in advance. A better solution should be investigated. Also, it should be noted that the change to AbstractTransactionServer is not necessarily incorrect. You should be able to use any timestamp LTE the last value reported by nextTimestamp() to obtain a read-only transaction. However, the semantics of obtaining a read-only transaction for a read-only transaction are distasteful/wasteful.

        Show
        bryanthompson bryanthompson added a comment - Committed revision 3804 provides a workaround for the exception thrown out of the BigdataSail when a read-only transaction identifier obtained from the lastCommitTime is passed to BigdataSail#getReadOnlyConnection(long). This code path will currently create a second read-only transaction. While this introduces uncessary latency into operations which already have a read-only transaction, it does permit correct functioning of the NanoSparqlServer - albeit by defeating an optimization in which the NanoSparqlServer was using a single read-only transaction to answer concurrent queries. For these reasons, this workaround is not ideal. One could have equally modified NanoSparqlServer to not obtain a transaction in advance. A better solution should be investigated. Also, it should be noted that the change to AbstractTransactionServer is not necessarily incorrect. You should be able to use any timestamp LTE the last value reported by nextTimestamp() to obtain a read-only transaction. However, the semantics of obtaining a read-only transaction for a read-only transaction are distasteful/wasteful.
        Hide
        bryanthompson bryanthompson added a comment -

        David wrote:

        The problems are described at the start of this thread. I repeat them here for ease of reference:

        >>com.bigdata.journal.TestAll (4 errors)
        >>IllegalStateException Timestamp is in future, at com.bigdata.journal.TesttransactionService.test_newTX_readOnly, line 666
        >>RuntimeException: No data for that commit time, at com.bigdata.journal.TesttransactionService.test_newTx_readOnly_timestampInFuture, line 912

        I sent you an e-mail about what I had done last week. I know that you were busy on other projects and so may have missed it. Here is a relevant extract:

        >>I have made two modifications to TestTransactionService which allow the tests to pass again. The first, in ?nextTimestamp()?, is legitimate I think. It >>ensures that the ?lastTimestamp? variable, introduced in revision 3804, is initialized and thus allows ?test_newTX_readOnly? to run successfully. I am >>concerned that the second change, in ?test_newTx_readOnly_timestampInFuture?, may not fully preserve the intent of the test.

        You will see from the last sentence of the extract that I was not sure about the seconds change. In a way, the test mechanism itself in 'test_newTx_readOnly_timestampInFuture? seemed at variance with change made in revision 3804. In fact, lacking a detailed understanding of AbstractTransactionService, I was having difficulty convincing myself about the change to 'assignTransactionIdentifier' in that revision. Consequently I was asking for your advice about the change to TestTransactionService further on in the e-mail from which the above extract is taken.

        Show
        bryanthompson bryanthompson added a comment - David wrote: The problems are described at the start of this thread. I repeat them here for ease of reference: >>com.bigdata.journal.TestAll (4 errors) >>IllegalStateException Timestamp is in future, at com.bigdata.journal.TesttransactionService.test_newTX_readOnly, line 666 >>RuntimeException: No data for that commit time, at com.bigdata.journal.TesttransactionService.test_newTx_readOnly_timestampInFuture, line 912 I sent you an e-mail about what I had done last week. I know that you were busy on other projects and so may have missed it. Here is a relevant extract: >>I have made two modifications to TestTransactionService which allow the tests to pass again. The first, in ?nextTimestamp()?, is legitimate I think. It >>ensures that the ?lastTimestamp? variable, introduced in revision 3804, is initialized and thus allows ?test_newTX_readOnly? to run successfully. I am >>concerned that the second change, in ?test_newTx_readOnly_timestampInFuture?, may not fully preserve the intent of the test. You will see from the last sentence of the extract that I was not sure about the seconds change. In a way, the test mechanism itself in 'test_newTx_readOnly_timestampInFuture? seemed at variance with change made in revision 3804. In fact, lacking a detailed understanding of AbstractTransactionService, I was having difficulty convincing myself about the change to 'assignTransactionIdentifier' in that revision. Consequently I was asking for your advice about the change to TestTransactionService further on in the e-mail from which the above extract is taken.
        Hide
        bryanthompson bryanthompson added a comment -

        I've updated the javadoc for ITransactionService#newTx(long) to indicate that the given timestamp may lie in the future. I've updated AbstractTransactionService to hand back nextTimestamp() for a read-only transaction request when the given timestamp is in the future. I've updated TestTransactionService to test this behavior for read-only and read-write tx (nothing had to be changed to support the latter). I've updated test_newTx_readOnly_timestampInFuture to request a timestamp which is known to be in the future and to verify that a read-only tx was assigned.

        These edits are self-consistent, but the tx semantics really ought to be reviewed in more depth, e.g., as part of https://sourceforge.net/apps/trac/bigdata/ticket/145 or when adding full-distributed read-write tx support to the database.

        Show
        bryanthompson bryanthompson added a comment - I've updated the javadoc for ITransactionService#newTx(long) to indicate that the given timestamp may lie in the future. I've updated AbstractTransactionService to hand back nextTimestamp() for a read-only transaction request when the given timestamp is in the future. I've updated TestTransactionService to test this behavior for read-only and read-write tx (nothing had to be changed to support the latter). I've updated test_newTx_readOnly_timestampInFuture to request a timestamp which is known to be in the future and to verify that a read-only tx was assigned. These edits are self-consistent, but the tx semantics really ought to be reviewed in more depth, e.g., as part of https://sourceforge.net/apps/trac/bigdata/ticket/145 or when adding full-distributed read-write tx support to the database.
        Hide
        bryanthompson bryanthompson added a comment -

        Closed. Not relevant to the new architecture.

        We have the information in the local and HA deployments to figure this stuff out without a further refactoring.

        Show
        bryanthompson bryanthompson added a comment - Closed. Not relevant to the new architecture. We have the information in the local and HA deployments to figure this stuff out without a further refactoring.

          People

          • Assignee:
            bryanthompson bryanthompson
            Reporter:
            bryanthompson bryanthompson
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: