[HADOOP-17912] Call destroy from input/output streams and refactor EncryptionAdapter #10
Conversation
Yetus run:============================================================================
|
HNS-OAuth[�[1;34mINFO�[m] Results: HNS-SharedKey[�[1;34mINFO�[m] Results: NonHNS-SharedKey[�[1;34mINFO�[m] Results: AppendBlob-HNS-OAuth[�[1;34mINFO�[m] Results: |
|
Assertion failure: |
|
Fix for breaking tests in saxenapranav@521d5bc, requesting you to please add in your PR. @mariosmeim-db |
| buffer = null; // de-reference the buffer so it can be GC'ed sooner | ||
| ReadBufferManager.getBufferManager().purgeBuffersForStream(this); | ||
| try { | ||
| if (encryptionAdapter != null) encryptionAdapter.destroy(); |
There was a problem hiding this comment.
Should we purgeBuffersForStream before destroying encryptionAdapter(populating 0s in contextKey)?
Reason:
- BufferWorkers run on parallel threads from the main thread.
- It can happen that we destroyed encryptionAdapter, and before purging the list, bufferWorker picks up buffer and starts to execute and it would fail as the keys would be wrong. Now, bufferWorker thread reaches https://github.com/mariosmeim-db/hadoop/blob/HADOOP-17912-databricks/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManager.java#L474, and then the thread-switches back to main thread, which purges inProgressQueue and completedQueue for the given stream related buffers. Then when bufferWorkerThread will get back to CPU, it would add buffer object in completedQueue and hence increasing heap-memory with time.
|
@pranavsaxena-microsoft I addressed your comments, let me know if they make sense. |
|
:::: AGGREGATED TEST RESULT :::: HNS-OAuth[INFO] Results: HNS-SharedKey[INFO] Results: NonHNS-SharedKey[INFO] Results: AppendBlob-HNS-OAuth[INFO] Results: |
Description of PR
This PR introduces the following changes:
EncryptionAdapterso thatcomputeKeysis a private method and is called internally from the adapter instead of publicly (e.g., fromAbfsClient). This also removes the need for storing the encoded key and sha strings (which are not destroyed anyway).EncryptionAdapter.destroy()EncryptionAdapterobject that is created inAbfsClient. addEncryptionKeyRequestHeadersis destroyed if it was created for an existing file, right after retrieving the key and shadestroyon the encryption adapter inAbfsInputStream.close()andAbfsOutputStream.close()How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?