Skip to content

IGNITE-15402 Drop partition & table folder deletion. #402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
ibessonov merged 13 commits into from
Oct 20, 2021
Merged

IGNITE-15402 Drop partition & table folder deletion. #402

ibessonov merged 13 commits into from
Oct 20, 2021

Conversation

@ibessonov
Copy link
Contributor

@ibessonov ibessonov commented Oct 18, 2021

@ibessonov
IGNITE-15402 Drop partition & table folder deletion.
a028d5e 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-15402 port range rolled back
62eeae0 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
sashapolo
sashapolo reviewed Oct 19, 2021
View reviewed changes
...ge-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java Outdated

Collections.reverse(copy);

IgniteUtils.closeAll(concat(
Copy link
Contributor

@sashapolo sashapolo Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said before, I think that simply adding everything into a list and then reversing it (or using a reverse iterator) might be a better choice here. This code is more complicated than it needs to be.

Copy link
Contributor Author

@ibessonov ibessonov Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding atomic reference array into list won't look good either. I'll rewrite this method anyway

...ge-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java
stop();

ColumnFamily partitionCf = partitionCfs.get(partId);
IgniteUtils.deleteIfExists(tablePath);
Copy link
Contributor

@sashapolo sashapolo Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the TableStorage should also create the tablePath since it deletes it?

Copy link
Contributor Author

@ibessonov ibessonov Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean "mkdir" in start method or literally determining location for the storage?

Copy link
Contributor

@sashapolo sashapolo Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mkdir

Copy link
Contributor Author

@ibessonov ibessonov Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@ibessonov
IGNITE-15402 review
e95cd8d 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-15402 More fixes
3394553 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
sashapolo
sashapolo reviewed Oct 19, 2021
View reviewed changes
examples/src/test/java/org/apache/ignite/example/sql/jdbc/SqlExamplesTest.java
@BeforeEach
@AfterEach
private void removeWorkDir() {
public void removeWorkDir() {
Copy link
Contributor

@sashapolo sashapolo Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we delete this method?

Copy link
Contributor Author

@ibessonov ibessonov Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it'll break the test, I tried. This directory is used by examples code.

...ge-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java Outdated
}

/** {@inheritDoc} */
@Override public synchronized void start() throws StorageException {
Copy link
Contributor

@sashapolo sashapolo Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this method synchronized?

Copy link
Contributor Author

@ibessonov ibessonov Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was for safety when I debugged JVM crashes. I think I'll remove these modifiers

@ibessonov
IGNITE-15402 raft groups stopping
ed56527 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-15402 assertion removed
c549af6 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-15402 don't drop partition if it's still yours
e270a68 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-15402 checkstyle
9b66433 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-15402 review
c9d2171 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-15402 review+
6e019ff 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov
IGNITE-15402 cleanup
c824f59 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
sashapolo
sashapolo reviewed Oct 20, 2021
View reviewed changes
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

/** {@inheritDoc} */
@Override public void stop() {
NamedListView<TableView> tablesView = tablesCfg.tables().value();
Copy link
Contributor

@sashapolo sashapolo Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add some comments about what is happening here?

Copy link
Contributor Author

@ibessonov ibessonov Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

modules/storage-api/src/main/java/org/apache/ignite/internal/storage/PartitionStorage.java
*
* @return Partition id.
*/
public int partitionId();
Copy link
Contributor

@sashapolo sashapolo Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this method?

Copy link
Contributor Author

@ibessonov ibessonov Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just makes sense and comes handy while debugging :)

@ibessonov
IGNITE-15402 few comments
b006481 
Signed-off-by: ibessonov <bessonov.ip@gmail.com>
@ibessonov ibessonov merged commit 723bdcc into apache:main Oct 20, 2021
@ibessonov ibessonov deleted the ignite-15402 branch October 20, 2021 11:50
AMashenkov pushed a commit that referenced this pull request Oct 21, 2021
@ibessonov @AMashenkov
IGNITE-15402 Partition drop in storage & table folder deletion. (#402)
dc30df6
ygerzhedovich pushed a commit that referenced this pull request Feb 23, 2024
@lowka
GG-37966: Sql. Remove link to IGNITE-19726 (#402)
6316271
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sashapolo sashapolo sashapolo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ibessonov @sashapolo