Details

    • Type: Task
    • Status: Done
    • Resolution: Done
    • Affects Version/s: BIGDATA_RELEASE_1_5_0
    • Fix Version/s: None
    • Component/s: Journal

      Description

      There are some instanceof tests in the code base around standalone, HA, and federation (at least 30 as "instanceof Journal"). Some of these instanceof tests are written to Journal when they should be written to IJournal. This ticket is for a code review of these tests. Now that we are using group commit, some of these are errors. For example, this was the root cause for BLZG-1171.

      The following need to be checked in more depth:
      - ProgramTask

      See BLZG-1171 (Concurrent modification problem with group commit)
      See BLZG-670 (Group commit)

        Activity

        Hide
        bryanthompson bryanthompson added a comment -

        BigdataSail. Interestingly, the tests on Journal vs IJournal are actually allowing the code to succeed. What happens is that an unisolated AbstractTask that is submitted through the ConcurrencyManager on the Journal is an IJournal (an IsolatedActionJournal) but not a Journal. Thus the getUnisolatedConnection() code does not attempt to obtain the global semaphore. This allows concurrent execution of tasks requiring unisolated connections for distinct namespaces on the same journal.

                boolean acquiredConnection = false;
                Lock writeLock = null;
                BigdataSailConnection conn = null;
                try {
                    if (getDatabase().getIndexManager() instanceof Journal) {
                        // acquire permit from Journal.
                        ((Journal) getDatabase().getIndexManager())
                                .acquireUnisolatedConnection();
                        acquiredConnection = true;
                    }
        

        I have added comments about this into the code and also updated the documentation on the BigdataSail regarding the concurrency model.

        Show
        bryanthompson bryanthompson added a comment - BigdataSail. Interestingly, the tests on Journal vs IJournal are actually allowing the code to succeed. What happens is that an unisolated AbstractTask that is submitted through the ConcurrencyManager on the Journal is an IJournal (an IsolatedActionJournal) but not a Journal. Thus the getUnisolatedConnection() code does not attempt to obtain the global semaphore. This allows concurrent execution of tasks requiring unisolated connections for distinct namespaces on the same journal. boolean acquiredConnection = false; Lock writeLock = null; BigdataSailConnection conn = null; try { if (getDatabase().getIndexManager() instanceof Journal) { // acquire permit from Journal. ((Journal) getDatabase().getIndexManager()) .acquireUnisolatedConnection(); acquiredConnection = true; } I have added comments about this into the code and also updated the documentation on the BigdataSail regarding the concurrency model.
        Hide
        bryanthompson bryanthompson added a comment -

        I also found code in TestNSSHealthCheck and TestNanoSparqlServer that was using the non-group-commit-aware versions of creating and destroying a KB. Further, TestNanoSparqlServer was not integrated into the CI test suite.

        Show
        bryanthompson bryanthompson added a comment - I also found code in TestNSSHealthCheck and TestNanoSparqlServer that was using the non-group-commit-aware versions of creating and destroying a KB. Further, TestNanoSparqlServer was not integrated into the CI test suite.
        Hide
        bryanthompson bryanthompson added a comment -

        The tests in AbstractBTree and Node for pre-fetch are part of code that is disabled by default or not live. These tests are correct as written since getReadExecutor() is not defined for IJournal. If we want to support the read executor pre-fetch pattern then the code needs to be updated to use IJournal and IJournal needs to expose getReadExecutor. I have added comments to this effect in the code.

        AbstractBTree.java::
            AbstractNode<?> loadChild(final Node parent, final int index) {
        
                if (false && store instanceof Journal
                        && ((Journal) store).getReadExecutor() != null) {
        
        Node.java::
                                    if ((btree.store instanceof Journal)
                                            && (((Journal) btree.store)
                                                    .getReadExecutor() != null)) {
        
                                        /*
                                         * Prefetch any child leaves we need to visit
                                         * and prefetch the right sibling of the node we
                                         * are about to visit if the iterator will span
                                         * that node as well.
                                         */
                                        prefetchChildLeaves((Node) child, fromKey,
                                                toKey);
        
                                    }
        
        Show
        bryanthompson bryanthompson added a comment - The tests in AbstractBTree and Node for pre-fetch are part of code that is disabled by default or not live. These tests are correct as written since getReadExecutor() is not defined for IJournal. If we want to support the read executor pre-fetch pattern then the code needs to be updated to use IJournal and IJournal needs to expose getReadExecutor. I have added comments to this effect in the code. AbstractBTree.java:: AbstractNode<?> loadChild(final Node parent, final int index) { if (false && store instanceof Journal && ((Journal) store).getReadExecutor() != null) { Node.java:: if ((btree.store instanceof Journal) && (((Journal) btree.store) .getReadExecutor() != null)) { /* * Prefetch any child leaves we need to visit * and prefetch the right sibling of the node we * are about to visit if the iterator will span * that node as well. */ prefetchChildLeaves((Node) child, fromKey, toKey); }
        Hide
        bryanthompson bryanthompson added a comment -

        Commit f9e8759caf7098bc30a95679b8f8e034088d0c8a.

        Closed pending CI results to the contrary.

        Show
        bryanthompson bryanthompson added a comment - Commit f9e8759caf7098bc30a95679b8f8e034088d0c8a. Closed pending CI results to the contrary.
        Hide
        bryanthompson bryanthompson added a comment -

        I have also done a code review on "instanceof AbstractJournal". This uncovered two instances that should be corrected.

        SD.describeOtherFeatures()

                {
        
                    final IIndexManager indexManager = tripleStore.getIndexManager();
        
                    if (indexManager instanceof IJournal) {
        
                        final IJournal jnl = (IJournal) indexManager;
        
                        final Quorum<HAGlue, QuorumService<HAGlue>> quorum = jnl
                                .getQuorum();
                        
                        if (quorum != null) {
        
                            final int k = quorum.replicationFactor();
                            
                            g.add(aService, SD.ReplicationFactor, tripleStore
                                    .getValueFactory().createLiteral(k));
        
                            g.add(aService, SD.feature, HighlyAvailable);
        
                        }
        
                    } else if (indexManager instanceof IBigdataFederation) {
        
                        g.add(aService, SD.feature, ScaleOut);
        
                    }
        
                }
        

        BigdataSail::

            /**
             * Return the {@link ITransactionService}.
             */
        	protected ITransactionService getTxService() {
        
        		final IIndexManager indexManager = database.getIndexManager();
        
        		final ITransactionService txService;
        
        		if (indexManager instanceof IJournal) {
        
        			txService = ((IJournal) indexManager).getLocalTransactionManager()
        					.getTransactionService();
        
        		} else {
        
        			txService = ((AbstractFederation<?>) indexManager)
        					.getTransactionService();
        
        		}
        
        		return txService;
        
        	} 
        

        I have run the NSS and TestBigdataSailWithQuads suites on this change. Committing to CI. cba0b2c62fae58f7c69d040f00188a49689262ac

        Show
        bryanthompson bryanthompson added a comment - I have also done a code review on "instanceof AbstractJournal". This uncovered two instances that should be corrected. SD.describeOtherFeatures() { final IIndexManager indexManager = tripleStore.getIndexManager(); if (indexManager instanceof IJournal) { final IJournal jnl = (IJournal) indexManager; final Quorum<HAGlue, QuorumService<HAGlue>> quorum = jnl .getQuorum(); if (quorum != null) { final int k = quorum.replicationFactor(); g.add(aService, SD.ReplicationFactor, tripleStore .getValueFactory().createLiteral(k)); g.add(aService, SD.feature, HighlyAvailable); } } else if (indexManager instanceof IBigdataFederation) { g.add(aService, SD.feature, ScaleOut); } } BigdataSail:: /** * Return the {@link ITransactionService}. */ protected ITransactionService getTxService() { final IIndexManager indexManager = database.getIndexManager(); final ITransactionService txService; if (indexManager instanceof IJournal) { txService = ((IJournal) indexManager).getLocalTransactionManager() .getTransactionService(); } else { txService = ((AbstractFederation<?>) indexManager) .getTransactionService(); } return txService; } I have run the NSS and TestBigdataSailWithQuads suites on this change. Committing to CI. cba0b2c62fae58f7c69d040f00188a49689262ac

          People

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

            Dates

            • Created:
              Updated:
              Resolved: