Conversation
| */ | ||
| public class H2Tree extends BPlusTree<H2Row, H2Row> { | ||
| /** */ | ||
| public static final String IGNITE_THROTTLE_INLINE_SIZE_CALCULATION = "IGNITE_THROTTLE_INLINE_SIZE_CALCULATION"; |
There was a problem hiding this comment.
Why do not we place property name constant to IgniteSystemProperties class as usual? With a bit of description in javadoc.
There was a problem hiding this comment.
in some case we didnt ex: org.apache.ignite.internal.processors.cache.persistence.GridCacheDatabaseSharedManager#IGNITE_PDS_CHECKPOINT_TEST_SKIP_SYNC , i just dont want to have a lot of changes instead of master code here, do u insist of this change ?
There was a problem hiding this comment.
If the property is not going to be used by user it can be kept here. Otherwise it is better to move it.
...s/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/database/H2Tree.java
Outdated
Show resolved
Hide resolved
...s/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/opt/GridH2Table.java
Outdated
Show resolved
Hide resolved
| throws IgniteCheckedException{ | ||
| String sql = H2Utils.indexDropSql(schemaName, idxName, ifExists); | ||
|
|
||
| GridH2Table tbl = dataTableForIndex(schemaName, idxName); |
There was a problem hiding this comment.
Search can go here only through a particular schema tables. An equivalent of operation schema(schemaName).tables().stream().filter(d -> d.table().containsUserIndex(idxName)).findAny().get() can be used for it.
There was a problem hiding this comment.
assert ok, other honestly don`t understand.
There was a problem hiding this comment.
dataTableForIndex iterates through all tables, schema(schemaName).tables() returns only currently needed schema tables.
There was a problem hiding this comment.
catch it, but what`s the difference between for loop with break and stream ?
There was a problem hiding this comment.
I was concerned more about access to tables using schema(schemaName).tables. Used a stream syntax just for a one-liner in a comment. For vs stream does not look as a big deal for me here.
...est/java/org/apache/ignite/internal/processors/cache/index/DynamicIndexAbstractSelfTest.java
Outdated
Show resolved
Hide resolved
| cctx0.shared().database().checkpointReadLock(); | ||
|
|
||
| try { | ||
| ((GridH2IndexBase)idx0).destroy(rmIndex); |
There was a problem hiding this comment.
Why do we need to pass rmIndex parameter here? In what cases we expect rmIndex == false here? Can we pass true unconditionally?
There was a problem hiding this comment.
cause on node and further cache.stop we need to set false here, that by design and beyond the scope of this ticket. If we pass almost true we would have undeterminate pageStore exceptions here )
| /** */ | ||
| @Test | ||
| @WithSystemProperty(key = IGNITE_THROTTLE_INLINE_SIZE_CALCULATION, value = "1") | ||
| public void testInlineSizeChange() throws Exception { |
There was a problem hiding this comment.
Also it would be great to reproduce in tests problems related to possible index corruption and other artifacts possible before that patch.
There was a problem hiding this comment.
ticket will be soon, but it still beyond the scope of this ticket.
|
|
||
| assert tbl != null; | ||
|
|
||
| tbl.setRemoveIndexOnDestroy(true); |
There was a problem hiding this comment.
I am thinking about a race condition when an index is being dropped and a cache is stopping on deactivation concurrently. A following invalid scenario looks possible:
- On deactivation
rmIndexflag is set to false. - On index drop
rmIndexis set to true. Index is dropped. rmIndex == trueleads that all indexes are destroyed on deactivation.
There was a problem hiding this comment.
Also a method name setRemoveIndexOnDestroy (and a javadoc) becomes misleading as it is going to be used for destroying an index on drop.
There was a problem hiding this comment.
Ivan,
Cache stop and schema operations (e.g. index drop) causes PME and should be serialized.
All schema operations are queued via GridQueryProcessor.schemaOps.
Cache stop operation fall into GridQueryProcessor.onCacheStop0 where all schemaOps have cancelled right before
setRemoveIndexOnDestroy flag is changed (inside idx.unregisterCache() call).
Possibly, GridQueryProcessor should wait for active SchemaOperationManager.worker-s before idx.unregisterCache(), but this should be investigated.
There was a problem hiding this comment.
Possibly, GridQueryProcessor should wait for active SchemaOperationManager.worker-s before idx.unregisterCache(), but this should be investigated.
Will be good to check it. AFAIK, cancellation uses Thread.interrupt, which cannot cancel running setRemoveIndexOnDestroy method alone.
Also, I think that it would be great to separate an intent of removing indexes on per-operation basis rather than having a single boolean flag shared between different operations (cache stop, drop index).
There was a problem hiding this comment.
I wonder, why do we need to do fake "Drop table" on deactivation, though database "Shutdown" is called later?
4 participants
No description provided.