-
Notifications
You must be signed in to change notification settings - Fork 86
[fix] don't remove cache on complete, instead use scheduled cleaners #4078
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
Changes from 4 commits
c1a8210
3609d94
7f9d514
7850caf
9816562
5e01ed3
eef936b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,11 +79,14 @@ class PublishService extends OperationService { | |
| `[PUBLISH] Minimum replication reached for operationId: ${operationId}, ` + | ||
| `datasetRoot: ${datasetRoot}, completed: ${completedNumber}/${minAckResponses}`, | ||
| ); | ||
| const cachedData = | ||
| (await this.operationIdService.getCachedOperationIdData(operationId)) || null; | ||
| await this.markOperationAsCompleted( | ||
| operationId, | ||
| blockchain, | ||
| null, | ||
| cachedData, | ||
| this.completedStatuses, | ||
| { reuseExistingCache: true }, | ||
| ); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: UpdateService still removes cache on complete unlike PublishServiceThe PR's intent is to "don't remove cache on complete, instead use scheduled cleaners" as stated in the title. |
||
| await this.repositoryModuleManager.updateMinAcksReached(operationId, true); | ||
| this.logResponsesSummary(completedNumber, failedNumber); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| import { describe, it, beforeEach, afterEach } from 'mocha'; | ||
| import { expect } from 'chai'; | ||
| import sinon from 'sinon'; | ||
|
|
||
| import OperationIdCleanerCommand from '../../../src/commands/cleaners/operation-id-cleaner-command.js'; | ||
| import { | ||
| OPERATION_ID_COMMAND_CLEANUP_TIME_MILLS, | ||
| OPERATION_ID_FILES_FOR_REMOVAL_MAX_NUMBER, | ||
| OPERATION_ID_MEMORY_CLEANUP_TIME_MILLS, | ||
| OPERATION_ID_STATUS, | ||
| } from '../../../src/constants/constants.js'; | ||
|
|
||
| describe('OperationIdCleanerCommand', () => { | ||
| let clock; | ||
| let operationIdService; | ||
| let repositoryModuleManager; | ||
| let logger; | ||
| let command; | ||
|
|
||
| beforeEach(() => { | ||
| clock = sinon.useFakeTimers(new Date('2023-01-01T00:00:00Z').getTime()); | ||
|
|
||
| operationIdService = { | ||
| getOperationIdMemoryCacheSizeBytes: sinon.stub().returns(1024), | ||
| getOperationIdFileCacheSizeBytes: sinon.stub().resolves(2048), | ||
| removeExpiredOperationIdMemoryCache: sinon.stub().resolves(512), | ||
| removeExpiredOperationIdFileCache: sinon.stub().resolves(3), | ||
| }; | ||
|
|
||
| repositoryModuleManager = { | ||
| removeOperationIdRecord: sinon.stub().resolves(), | ||
| }; | ||
|
|
||
| logger = { | ||
| debug: sinon.spy(), | ||
| info: sinon.spy(), | ||
| warn: sinon.spy(), | ||
| error: sinon.spy(), | ||
| }; | ||
|
|
||
| command = new OperationIdCleanerCommand({ | ||
| logger, | ||
| repositoryModuleManager, | ||
| operationIdService, | ||
| fileService: {}, | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| clock.restore(); | ||
| }); | ||
|
|
||
| it('cleans memory with 1h TTL and files with 24h TTL while reporting footprint', async () => { | ||
| await command.execute(); | ||
|
|
||
| expect(operationIdService.getOperationIdMemoryCacheSizeBytes.calledOnce).to.be.true; | ||
| expect(operationIdService.getOperationIdFileCacheSizeBytes.calledOnce).to.be.true; | ||
|
|
||
| expect( | ||
| repositoryModuleManager.removeOperationIdRecord.calledWith( | ||
| Date.now() - OPERATION_ID_COMMAND_CLEANUP_TIME_MILLS, | ||
| [OPERATION_ID_STATUS.COMPLETED, OPERATION_ID_STATUS.FAILED], | ||
| ), | ||
| ).to.be.true; | ||
|
|
||
| expect( | ||
| operationIdService.removeExpiredOperationIdMemoryCache.calledWith( | ||
| OPERATION_ID_MEMORY_CLEANUP_TIME_MILLS, | ||
| ), | ||
| ).to.be.true; | ||
|
|
||
| expect( | ||
| operationIdService.removeExpiredOperationIdFileCache.calledWith( | ||
| OPERATION_ID_COMMAND_CLEANUP_TIME_MILLS, | ||
| OPERATION_ID_FILES_FOR_REMOVAL_MAX_NUMBER, | ||
| ), | ||
| ).to.be.true; | ||
|
|
||
| expect(logger.debug.called).to.be.true; | ||
| }); | ||
|
|
||
| it('handles missing memory cache gracefully', async () => { | ||
| operationIdService.getOperationIdMemoryCacheSizeBytes.throws(new Error('no memory cache')); | ||
| await command.execute(); | ||
|
|
||
| expect( | ||
| repositoryModuleManager.removeOperationIdRecord.calledWith( | ||
| Date.now() - OPERATION_ID_COMMAND_CLEANUP_TIME_MILLS, | ||
| [OPERATION_ID_STATUS.COMPLETED, OPERATION_ID_STATUS.FAILED], | ||
| ), | ||
| ).to.be.true; | ||
|
|
||
| expect( | ||
| operationIdService.removeExpiredOperationIdFileCache.calledWith( | ||
| OPERATION_ID_COMMAND_CLEANUP_TIME_MILLS, | ||
| OPERATION_ID_FILES_FOR_REMOVAL_MAX_NUMBER, | ||
| ), | ||
| ).to.be.true; | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| import { describe, it, beforeEach, afterEach } from 'mocha'; | ||
| import { expect } from 'chai'; | ||
| import fs from 'fs/promises'; | ||
| import path from 'path'; | ||
| import os from 'os'; | ||
| import OperationIdService from '../../../src/service/operation-id-service.js'; | ||
|
|
||
| describe('OperationIdService file cache cleanup', () => { | ||
| let tmpDir; | ||
| let service; | ||
|
|
||
| beforeEach(async () => { | ||
| tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'opid-cache-')); | ||
| const now = Date.now(); | ||
|
|
||
| // Older than TTL (2 hours) | ||
| const oldFile = path.join(tmpDir, 'old.json'); | ||
| await fs.writeFile(oldFile, '{}'); | ||
| await fs.utimes( | ||
| oldFile, | ||
| new Date(now - 2 * 60 * 60 * 1000), | ||
| new Date(now - 2 * 60 * 60 * 1000), | ||
| ); | ||
|
|
||
| // Newer than TTL (10 minutes) | ||
| const newFile = path.join(tmpDir, 'new.json'); | ||
| await fs.writeFile(newFile, '{}'); | ||
| await fs.utimes(newFile, new Date(now - 10 * 60 * 1000), new Date(now - 10 * 60 * 1000)); | ||
|
|
||
| const fileService = { | ||
| getOperationIdCachePath: () => tmpDir, | ||
| async pathExists(p) { | ||
| try { | ||
| await fs.stat(p); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| }, | ||
| readDirectory: (p) => fs.readdir(p), | ||
| stat: (p) => fs.stat(p), | ||
| removeFile: (p) => fs.rm(p, { force: true }), | ||
| }; | ||
|
|
||
| service = new OperationIdService({ | ||
| logger: { debug: () => {}, warn: () => {}, error: () => {} }, | ||
| fileService, | ||
| repositoryModuleManager: {}, | ||
| eventEmitter: { emit: () => {} }, | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await fs.rm(tmpDir, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| it('removes only files older than TTL', async () => { | ||
| const deleted = await service.removeExpiredOperationIdFileCache(60 * 60 * 1000, 10); | ||
| const remainingFiles = await fs.readdir(tmpDir); | ||
|
|
||
| expect(deleted).to.equal(1); | ||
| expect(remainingFiles).to.deep.equal(['new.json']); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are passing data we read from cache as responseData, why do we want to cache it again? Seems wasteful, perhaps we can remove this. Or split into 2 separate functions like we talked about