Skip to content
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.

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

Should we always call destory before removing a mapped file ? #2019

Open
imaffe opened this issue May 20, 2020 · 3 comments
Open

Should we always call destory before removing a mapped file ? #2019

imaffe opened this issue May 20, 2020 · 3 comments
Labels
no stale This will never be considered stale type/question

Comments

@imaffe
Copy link
Contributor

imaffe commented May 20, 2020

The issue tracker is ONLY used for bug report(feature request need to follow RIP process). Keep in mind, please check whether there is an existing same report before your raise a new one.

Alternately (especially if your communication is not a bug report), you can send mail to our mailing lists. We welcome any friendly suggestions, bug fixes, collaboration and other improvements.

Please ensure that your bug report is clear and that it is complete. Otherwise, we may be unable to understand it or to reproduce it, either of which would prevent us from fixing the bug. We strongly recommend the report(bug report or feature request) could include some hints as the following:

BUG REPORT

  1. Please describe the issue you observed:

In MappedFileQueue:273 we have

        while (iterator.hasPrevious()) {
            mappedFileLast = iterator.previous();
            if (offset >= mappedFileLast.getFileFromOffset()) {
                int where = (int) (offset % mappedFileLast.getFileSize());
                mappedFileLast.setFlushedPosition(where);
                mappedFileLast.setWrotePosition(where);
                mappedFileLast.setCommittedPosition(where);
                break;
            } else {
                 // Can we add destroy here ?
                 // mappedFileLast.destroy();
                iterator.remove();
            }
        }
  • What did you expect to see?

  • What did you see instead?

  1. Please tell us about your environment:

  2. Other information (e.g. detailed explanation, logs, related issues, suggestions how to fix, etc):

FEATURE REQUEST

  1. Please describe the feature you are requesting.

  2. Provide any additional detail on your proposed use case for this feature.

  3. Indicate the importance of this issue to you (blocker, must-have, should-have, nice-to-have). Are you currently using any workarounds to address this issue?

  4. If there are some sub-tasks using -[] for each subtask and create a corresponding issue to map to the sub task:

@RongtongJin
Copy link
Contributor

RocketMQ will delete the files periodically through configuration or disk is full. But if we call destory in this method, the file will be deleted and the messages will be lost. So I think this place we should not call the destory.

@imaffe
Copy link
Contributor Author

imaffe commented May 22, 2020

Sorry I didn't make it claer. Actually this is found in resetOffset():

    public boolean resetOffset(long offset) {
        MappedFile mappedFileLast = getLastMappedFile();

        if (mappedFileLast != null) {
            long lastOffset = mappedFileLast.getFileFromOffset() +
                mappedFileLast.getWrotePosition();
            long diff = lastOffset - offset;

            final int maxDiff = this.mappedFileSize * 2;
            if (diff > maxDiff)
                return false;
        }

        ListIterator<MappedFile> iterator = this.mappedFiles.listIterator();

        while (iterator.hasPrevious()) {
            mappedFileLast = iterator.previous();
            if (offset >= mappedFileLast.getFileFromOffset()) {
                int where = (int) (offset % mappedFileLast.getFileSize());
                mappedFileLast.setFlushedPosition(where);
                mappedFileLast.setWrotePosition(where);
                mappedFileLast.setCommittedPosition(where);
                break;
            } else {
                iterator.remove();
            }
        }
        return true;
    }

and I saws deleteLastFile() actually deleted the file.

    public void deleteLastMappedFile() {
        MappedFile lastMappedFile = getLastMappedFile();
        if (lastMappedFile != null) {
            lastMappedFile.destroy(1000);
            this.mappedFiles.remove(lastMappedFile);
            log.info("on recover, destroy a logic mapped file " + lastMappedFile.getFileName());
        }
    }

So it appears to me that when we remove a mappedFile from this.mappedFiles,we actually want to delete the underlying file as well. I thought if we use iterator.remove() without deleting the file, we lose the reference to that file as well. (because this.mappedFiles are supposed to hold the )

Otherwise it might be that we don't need to call iterator.remove(), because I saw Java 8 doc

(For CopyOnWriteArrayList iterators, in our case this.mappedFiles )Mutation operations on iterators (remove, set, and add) are not supported. These methods throw UnsupportedOperationException. (which is a runtime exception)

Which seems weird to me. If that's true then the iterator.remove() is not working at all.

@imaffe
Copy link
Contributor Author

imaffe commented Jun 17, 2020

The problem here is iterator.remove() doesn't really do anything (Unsupported Operation) Could be a potential bug. Will catch up with this one recently

@aaron-ai aaron-ai added the no stale This will never be considered stale label Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no stale This will never be considered stale type/question
Projects
None yet
Development

No branches or pull requests

3 participants