Summary:
FreeBSD uses jemalloc as the base malloc implementation.
The patch has been functional on FreeBSD as of the MariaDB 10.2 port.
Closes https://github.com/facebook/rocksdb/pull/3386
Differential Revision: D6765742
Pulled By: yiwu-arbug
fbshipit-source-id: d55dbc082eecf640ef3df9a21f26064ebe6587e8
Summary:
When blob_files is empty, std::min_element will return blobfiles.end(), which cannot be dereference. Fixing it.
Closes https://github.com/facebook/rocksdb/pull/3387
Differential Revision: D6764927
Pulled By: yiwu-arbug
fbshipit-source-id: 86f78700132be95760d35ac63480dfd3a8bbe17a
Summary:
Flush() call could be waiting indefinitely if min_write_buffer_number_to_merge is used. Consider the sequence:
1. User call Flush() with flush_options.wait = true
2. The manual flush started in the background
3. New memtable become immutable because of writes. The new memtable will not trigger flush if min_write_buffer_number_to_merge is not reached.
4. The manual flush finish.
Because of the new memtable created at step 3 not being flush, previous logic of WaitForFlushMemTable() keep waiting, despite the memtables it intent to flush has been flushed.
Here instead of checking if there are any more memtables to flush, WaitForFlushMemTable() also check the id of the earliest memtable. If the id is larger than that of latest memtable at the time flush was initiated, it means all the memtable at the time of flush start has all been flush.
Closes https://github.com/facebook/rocksdb/pull/3378
Differential Revision: D6746789
Pulled By: yiwu-arbug
fbshipit-source-id: 35e698f71c7f90b06337a93e6825f4ea3b619bfa
Summary:
We have seen cases where it could be good to change TTL on already open DB.
Change ttl in TtlCompactionFilterFactory on open db.
Next time a filter is created, it will filter accroding to the set TTL.
Is this something that could be useful for others?
Any downsides?
Closes https://github.com/facebook/rocksdb/pull/3292
Differential Revision: D6731993
Pulled By: miasantreble
fbshipit-source-id: 73b94d69237b11e8730734389052429d621a6b1e
Summary:
When calling `DisableFileDeletions` followed by `GetSortedWalFiles`, we guarantee the files returned by the latter call won't be deleted until after file deletions are re-enabled. However, `GetSortedWalFiles` didn't omit files already planned for deletion via `PurgeObsoleteFiles`, so the guarantee could be broken.
We fix it by making `GetSortedWalFiles` wait for the number of pending purges to hit zero if file deletions are disabled. This condition is eventually met since `PurgeObsoleteFiles` is guaranteed to be called for the existing pending purges, and new purges cannot be scheduled while file deletions are disabled. Once the condition is met, `GetSortedWalFiles` simply returns the content of DB and archive directories, which nobody can delete (except for deletion scheduler, for which I plan to fix this bug later) until deletions are re-enabled.
Closes https://github.com/facebook/rocksdb/pull/3341
Differential Revision: D6681131
Pulled By: ajkr
fbshipit-source-id: 90b1e2f2362ea9ef715623841c0826611a817634
Summary:
After af92d4ad11, only exclusive manual compaction can have conflict. dc360df81e updated the conflict-checking test case accordingly. But we missed the point that exclusive manual compaction can only conflict with automatic compactions scheduled after it, since it waits on pending automatic compactions before it begins running.
This PR updates the test case to ensure the automatic compactions are scheduled after the manual compaction starts but before it finishes, thus ensuring a conflict. I also cleaned up the test case to use less space as I saw it cause out-of-space error on travis.
Closes https://github.com/facebook/rocksdb/pull/3375
Differential Revision: D6735162
Pulled By: ajkr
fbshipit-source-id: 020530a4e150a4786792dce7cec5d66b420cb884
Summary:
* Fix DBTest.CompactRangeWithEmptyBottomLevel lite build failure
* Fix DBTest.AutomaticConflictsWithManualCompaction failure introduce by #3366
* Fix BlockBasedTableTest::IndexUncompressed should be disabled if snappy is disabled
* Fix ASAN failure with DBBasicTest::DBClose test
Closes https://github.com/facebook/rocksdb/pull/3373
Differential Revision: D6732313
Pulled By: yiwu-arbug
fbshipit-source-id: 1eb9b9d9a8d795f56188fa9770db9353f6fdedc5
Summary:
Issue #3370 Simple fixes to make RocksDB project working also as a submodule of other bigger one.
Closes https://github.com/facebook/rocksdb/pull/3372
Differential Revision: D6729595
Pulled By: ajkr
fbshipit-source-id: eee2589e7a7c4322873dff8510eebd050301c54c
Summary:
If there's manual compaction in the queue, then "HaveManualCompaction(compaction_queue_.front())" will return true, and this cause too frequent MaybeScheduleFlushOrCompaction().
https://github.com/facebook/rocksdb/issues/3198
Closes https://github.com/facebook/rocksdb/pull/3366
Differential Revision: D6729575
Pulled By: ajkr
fbshipit-source-id: 96da04f8fd33297b1ccaec3badd9090403da29b0
Summary:
Currently, the only way to close an open DB is to destroy the DB
object. There is no way for the caller to know the status. In one
instance, the destructor encountered an error due to failure to
close a log file on HDFS. In order to prevent silent failures, we add
DB::Close() that calls CloseImpl() which must be implemented by its
descendants.
The main failure point in the destructor is closing the log file. This
patch also adds a Close() entry point to Logger in order to get status.
When DBOptions::info_log is allocated and owned by the DBImpl, it is
explicitly closed by DBImpl::CloseImpl().
Closes https://github.com/facebook/rocksdb/pull/3348
Differential Revision: D6698158
Pulled By: anand1976
fbshipit-source-id: 9468e2892553eb09c4c41b8723f590c0dbd8ab7d
Summary:
Java static builds are again broken, this time due Snappy version upgrade introduced in 90c1d81975 (#3331).
This is due to two reasons:
1. The new Snappy packages should now be downloaded from https://github.com/google/snappy/archive/<pkg.tar.gz> instead of https://github.com/google/snappy/releases/download/<pkg.tar.gz> which we are using now.
1. In addition to the the above URL change, Snappy changed its build from using autotools to CMake based : e69d9f8806/README.md (L65-L72)
So more changes are needed if we are going to upgrade to 1.1.7. Hence reverting the version upgrade until we figure them out.
Closes https://github.com/facebook/rocksdb/pull/3363
Differential Revision: D6716983
Pulled By: sagar0
fbshipit-source-id: f451a1bc5eb0bb090f4da07bc3e5ba72cf89aefa
Summary:
Split `JobContext::HaveSomethingToDelete` into two functions: itself and `JobContext::HaveSomethingToClean`. Now we won't call `DBImpl::PurgeObsoleteFiles` in cases where we really just need to call `JobContext::Clean`. The change is needed because I want to track pending calls to `PurgeObsoleteFiles` for a bug fix, which is much simpler if we only call it after `FindObsoleteFiles` finds files to delete.
Closes https://github.com/facebook/rocksdb/pull/3350
Differential Revision: D6690609
Pulled By: ajkr
fbshipit-source-id: 61502e7469288afe16a663a1b7df345baeaf246f
Summary:
This test often causes out-of-space error when run on travis. We don't want such stress tests in our unit test suite.
The bug in #596, which this test intends to expose, can be repro'd as long as the bottommost level(s) are empty when CompactRange is called. I rewrote the test to cover this simple case without writing a lot of data.
Closes https://github.com/facebook/rocksdb/pull/3362
Differential Revision: D6710417
Pulled By: ajkr
fbshipit-source-id: 9a1ec85e738c813ac2fee29f1d5302065ecb54c5
Summary:
Java build on PPC64le has been broken since a few months, due to #2716. Fixing it with the least amount of changes.
(We should cleanup a little around this code when time permits).
This should fix the build failures seen in http://140.211.168.68:8080/job/Rocksdb/ .
Closes https://github.com/facebook/rocksdb/pull/3359
Differential Revision: D6712938
Pulled By: sagar0
fbshipit-source-id: 3046e8f072180693de2af4762934ec1ace309ca4
Summary:
I installed the ruby dependencies and ran `bundle update nokogiri`. It depends on a newer version of "mini_portile2" which I missed in 9c2f64e148. Now `bundle install` works again.
Closes https://github.com/facebook/rocksdb/pull/3361
Differential Revision: D6710164
Pulled By: ajkr
fbshipit-source-id: 9a08d6cc6400ef495b715b3d68b04ce3f3367031
Summary:
Hello and thank you for RocksDB,
While looking into the buffered io used when an `OPTIONS` file is read I noticed the `OPTIONS` files produced by RocksDB 5.8.8 (and head of master) were just over 4096 bytes in size, resulting in the version of glibc I am using (glibc-2.17-196.el7) (on the filesystem used) being passed a 4K buffer for the `fread_unlocked` call and 2 system call reads using a 4096 buffer being used to read the contents of the `OPTIONS` file.
If the buffer size is increased to 8192 then 1 system call read is used to read the contents.
As I think the buffer size is just used for reading `OPTIONS` files, and I thought it likely that `OPTIONS` files have increased in size (as more options are added), I thought I would suggest an increase.
[ If the comments from the top of the `OPTIONS` file are removed, and white space from the start of lines is removed then the size can be reduced to be under 4K, but as more options are added the size seems likely to grow again. ]
Create a new database:
```
> ./ldb --create_if_missing --db=/tmp/rdb_tmp put 1 1
OK
```
The OPTIONS file is 4252 bytes:
```
> stat /tmp/rdb_tmp/OPTIONS* | head -n 2
File: ‘/tmp/rdb_tmp/OPTIONS-000005’
Size: 4252 Blocks: 16 IO Block: 4096 regular file
```
Before, the 4096 byte buffer is used from 2 system read calls:
```
> strace -f ./ldb --try_load_options --db=/tmp/rdb_tmp get DOES_NOT_EXIST 2>&1 |
grep -A 1 'RocksDB option file'
read(3, "# This is a RocksDB option file."..., 4096) = 4096
read(3, "e\n metadata_block_size=4096\n c"..., 4096) = 156
```
ltrace shows 4096 passed to fread_unlocked
```
> ltrace -S -f ./ldb --try_load_options --db=/tmp/rdb_tmp get DOES_NOT_EXIST 2>&1 |
grep -C 3 'RocksDB option file'
[pid 51013] fread_unlocked(0x7ffd5fbf2d50, 1, 4096, 0x7fd2e084e780 <unfinished ...>
[pid 51013] fstat@SYS(3, 0x7ffd5fbf28f0) = 0
[pid 51013] mmap@SYS(nil, 4096, 3, 34, -1, 0) = 0x7fd2e318c000
[pid 51013] read@SYS(3, "# This is a RocksDB option file."..., 4096) = 4096
[pid 51013] <... fread_unlocked resumed> ) = 4096
...
```
After, the 8192 byte buffer is used from 1 system read call:
```
> strace -f ./ldb --try_load_options --db=/tmp/rdb_tmp get DOES_NOT_EXIST 2>&1 | grep -A 1 'RocksDB option file'
read(3, "# This is a RocksDB option file."..., 8192) = 4252
read(3, "", 4096) = 0
```
ltrace shows 8192 passed to fread_unlocked
```
> ltrace -S -f ./ldb --try_load_options --db=/tmp/rdb_tmp get DOES_NOT_EXIST 2>&1 | grep -C 3 'RocksDB option file'
[pid 146611] fread_unlocked(0x7ffcfba382f0, 1, 8192, 0x7fc4e844e780 <unfinished ...>
[pid 146611] fstat@SYS(3, 0x7ffcfba380f0) = 0
[pid 146611] mmap@SYS(nil, 4096, 3, 34, -1, 0) = 0x7fc4eaee0000
[pid 146611] read@SYS(3, "# This is a RocksDB option file."..., 8192) = 4252
[pid 146611] read@SYS(3, "", 4096) = 0
[pid 146611] <... fread_unlocked resumed> ) = 4252
[pid 146611] feof(0x7fc4e844e780) = 1
```
Closes https://github.com/facebook/rocksdb/pull/3294
Differential Revision: D6653684
Pulled By: ajkr
fbshipit-source-id: 222f25f5442fefe1dcec18c700bd9e235bb63491
Summary:
to save a string copy for some use cases.
The change is pretty straightforward, please feel free to let me know if you want to suggest any tests for it.
Closes https://github.com/facebook/rocksdb/pull/3349
Differential Revision: D6706828
Pulled By: yiwu-arbug
fbshipit-source-id: 873ce4442937bdc030b395c7f99228eda7f59eb7
Summary:
Re-use metadata for reading Compression Dictionary on BlockBased
table open, this saves two reads from disk.
This helps to our 999 percentile in 5.6.1 where prefetch buffer is not present.
Closes https://github.com/facebook/rocksdb/pull/3354
Differential Revision: D6695753
Pulled By: ajkr
fbshipit-source-id: bb8acd9e9e66e65b89c548ab8940570ae360333c
Summary:
It was using the same directory as `db_options_test` so transiently failed when unit tests were run in parallel.
Closes https://github.com/facebook/rocksdb/pull/3352
Differential Revision: D6691649
Pulled By: ajkr
fbshipit-source-id: bee433484fec4faedd5cadf2db3c92fdcc99a170
Summary:
- Change directory name from "db_test" to "checkpoint_test". Previously it used the same directory as `db_test`
- Systematically cleanup snapshot and snapshot staging directories before each test. Previously a failed test run caused subsequent runs to fail, particularly when the first failure caused "snapshot.tmp" to not be cleaned up.
Closes https://github.com/facebook/rocksdb/pull/3351
Differential Revision: D6691015
Pulled By: ajkr
fbshipit-source-id: 4fc2ac2e21ff2617ea0e96297c5132b5f2eefd79
Summary:
Most popular versions of GCC can't identify platform on ARM if "-march=native" is specified. Remove it to unblock most people.
Closes https://github.com/facebook/rocksdb/pull/3346
Differential Revision: D6690544
Pulled By: siying
fbshipit-source-id: bbaba9fe2645b6b37144b36ea75beeff88992b49
Summary:
I experienced weird segfault because of this mismatch of type in log formatting. Fix it.
Closes https://github.com/facebook/rocksdb/pull/3345
Differential Revision: D6687224
Pulled By: siying
fbshipit-source-id: c51fb1c008b7ebc3efdc353a4adad3e8f5b3e9de
Summary:
got confused while reading `FindObsoleteFiles` due to thinking it's a local variable, so renamed it properly
Closes https://github.com/facebook/rocksdb/pull/3342
Differential Revision: D6684797
Pulled By: ajkr
fbshipit-source-id: a4df0aae1cccce99d4dd4d164aadc85b17707132
Summary:
The macro was added by mistake in #2372
Closes https://github.com/facebook/rocksdb/pull/3343
Differential Revision: D6681356
Pulled By: yiwu-arbug
fbshipit-source-id: 4180172fb0eaef4189c07f219241e0c261c03461
Summary:
This patch addresses a couple of minor TODOs for WritePrepared Txn such as double checking some assert statements at runtime as well, skip extra AddPrepared in non-2pc transactions, and safety check for infinite loops.
Closes https://github.com/facebook/rocksdb/pull/3302
Differential Revision: D6617002
Pulled By: maysamyabandeh
fbshipit-source-id: ef6673c139cb49f64c0879508d2f573b78609aca
Summary:
Previously on a blob db read, we are making a read of the blob value, and then make another read to get CRC checksum. I'm combining the two read into one.
readrandom db_bench with 1G database with base db size of 13M, value size 1k:
`./db_bench --db=/home/yiwu/tmp/db_bench --use_blob_db --value_size=1024 --num=1000000 --benchmarks=readrandom --use_existing_db --cache_size=32000000`
master: throughput 234MB/s, get micros p50 5.984 p95 9.998 p99 20.817 p100 787
this PR: throughput 261MB/s, get micros p50 5.157 p95 9.928 p99 20.724 p100 190
Closes https://github.com/facebook/rocksdb/pull/3301
Differential Revision: D6615950
Pulled By: yiwu-arbug
fbshipit-source-id: 052410c6d8539ec0cc305d53793bbc8f3616baa3
Summary:
With the ZSTD dictionary generator support added in #3057
`PORTABLE=1 ROCKSDB_NO_FBCODE=1 make rocksdbjavastatic` fails as it can't find zdict.h. Specifically due to:
e3a06f12d2/util/compression.h (L39)
In java static builds zstd code gets directly downloaded from https://github.com/facebook/zstd , and in there zdict.h is under dictBuilder directory. So, I modified libzstd.a target to use `make install` to collect all the header files into a single location and used that as the zstd's include path.
Closes https://github.com/facebook/rocksdb/pull/3260
Differential Revision: D6669850
Pulled By: sagar0
fbshipit-source-id: f8a7562a670e5aed4c4fb6034a921697590d7285
Summary:
Make dependacies switches compatible with other OS builds
TODO: Make find_package work for Windows.
Closes https://github.com/facebook/rocksdb/pull/3322
Differential Revision: D6667637
Pulled By: sagar0
fbshipit-source-id: 5afcd7bbfe69465310a4fbc8e589f01e506b95f5
Summary:
DestroyDB that is used in tests loops over the files returned by ::GetChildren and delete them one by one. Such files might be already deleted in the file system (during DeleteObsoleteFileImpl for example) but will get actually deleted with a delay sometimes before ::DeleteFile is called on the file name. We have some test failures where FaultInjectionTestEnv::DeleteFile fails on assert(s.ok()) during DestroyDB. This patch removes the assert statement to fix that.
Closes https://github.com/facebook/rocksdb/pull/3324
Differential Revision: D6659545
Pulled By: maysamyabandeh
fbshipit-source-id: 4c9552fbcd494dcf3e61d475c11fc965c4388b2c
Summary:
added support for C and asm files as required for e612e31740.
Closes https://github.com/facebook/rocksdb/pull/3299
Differential Revision: D6612479
Pulled By: ajkr
fbshipit-source-id: 6263ed7c1602f249460421825c76b5721f396163
Summary:
BlockTest.BlockReadAmpBitmap is too slow and times out in some environments. Speed it up by:
(1) improve the way the verification is done. With this it is 5 times faster
(2) run fewer tests for large blocks. This cut it down by another 10 times.
Now it can finish in similar time as other tests.
Closes https://github.com/facebook/rocksdb/pull/3313
Differential Revision: D6643711
Pulled By: siying
fbshipit-source-id: c2397d666eab5421a78ca87e1e45491e0f832a6d
Summary:
FILE_FLAG_WRITE_THROUGH is for disabling device on-board cache in windows API, which should be disabled if user doesn't need system cache.
There was a perf issue related with this, we found during memtable flush, the high percentile latency jumps significantly. During profiling, we found those high latency (P99.9) read requests got queue-jumped by write requests from memtable flush and takes 80ms or even more time to wait, even when SSD overall IO throughput is relatively low.
After enabling FILE_FLAG_WRITE_THROUGH, we rerun the test found high percentile latency drops a lot without observable impact on writes.
Scenario 1: 40MB/s + 40MB/s R/W compaction throughput
Original | FILE_FLAG_WRITE_THROUGH | Percentage reduction
---------------------------------------------------------------
P99.9 | 56.897 ms | 35.593 ms | -37.4%
P99 | 3.905 ms | 3.896 ms | -2.8%
Scenario 2: 14MB/s + 14MB/s R/W compaction throughput, cohosted with 100+ other rocksdb instances have manually triggered memtable flush operations (memtable is tiny), creating a lot of randomized the small file writes operations during test.
Original | FILE_FLAG_WRITE_THROUGH | Percentage reduction
---------------------------------------------------------------
P99.9 | 86.227 ms | 50.436 ms | -41.5%
P99 | 8.415 ms | 3.356 ms | -60.1%
Closes https://github.com/facebook/rocksdb/pull/3225
Differential Revision: D6624174
Pulled By: miasantreble
fbshipit-source-id: 321b86aee9d74470840c70e5d0d4fa9880660a91
Summary:
Fixes the following ASAN error:
```
==2108042==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7fc50ae9b868 at pc 0x7fc5112aff55 bp 0x7fff9eb9dc10 sp 0x7fff9eb9dc08
=== How to use this, how to get the raw stack trace, and more: fburl.com/ASAN ===
READ of size 8 at 0x7fc50ae9b868 thread T0
SCARINESS: 23 (8-byte-read-stack-use-after-scope)
#0 rocksdb/dbformat.h:164 rocksdb::InternalKeyComparator::user_comparator() const
#1 librocksdb_src_rocksdb_lib.so+0x1429a7d rocksdb::RangeDelAggregator::InitRep(std::vector<...> const&)
#2 librocksdb_src_rocksdb_lib.so+0x142ceae rocksdb::RangeDelAggregator::AddTombstones(std::unique_ptr<...>)
#3 librocksdb_src_rocksdb_lib.so+0x1382d88 rocksdb::ForwardIterator::RebuildIterators(bool)
#4 librocksdb_src_rocksdb_lib.so+0x1382362 rocksdb::ForwardIterator::ForwardIterator(rocksdb::DBImpl*, rocksdb::ReadOptions const&, rocksdb::ColumnFamilyData*, rocksdb::SuperVersion*)
#5 librocksdb_src_rocksdb_lib.so+0x11f433f rocksdb::DBImpl::NewIterator(rocksdb::ReadOptions const&, rocksdb::ColumnFamilyHandle*)
#6 rocksdb/src/include/rocksdb/db.h:382 rocksdb::DB::NewIterator(rocksdb::ReadOptions const&)
#7 rocksdb/db_range_del_test.cc:807 rocksdb::DBRangeDelTest_TailingIteratorRangeTombstoneUnsupported_Test::TestBody()
#18 rocksdb/db_range_del_test.cc:1006 main
Address 0x7fc50ae9b868 is located in stack of thread T0 at offset 104 in frame
#0 librocksdb_src_rocksdb_lib.so+0x13825af rocksdb::ForwardIterator::RebuildIterators(bool)
```
Closes https://github.com/facebook/rocksdb/pull/3300
Differential Revision: D6612989
Pulled By: ajkr
fbshipit-source-id: e7ea2ed914c1b80a8a29d71d92440a6bd9cbcc80
Summary:
Blog post to introduce the next generation of transaction engine at RocksDB.
Closes https://github.com/facebook/rocksdb/pull/3296
Differential Revision: D6612932
Pulled By: maysamyabandeh
fbshipit-source-id: 5bfa91ce84e937f5e4346bbda5a4725d0a7fd131
Summary:
When there is a background error PreprocessWrite returns without marking the logs synced. If we keep need_log_sync to true, it would try to sync them at the end, which would break the logic. The patch would unset need_log_sync if the logs end up not being marked for sync in PreprocessWrite.
Closes https://github.com/facebook/rocksdb/pull/3293
Differential Revision: D6602347
Pulled By: maysamyabandeh
fbshipit-source-id: 37ee04209e8dcfd78de891654ce50d0954abeb38
Summary:
This fixes the following warnings when compiled with GCC7:
util/transaction_test_util.cc: In static member function ‘static rocksdb::Status rocksdb::RandomTransactionInserter::DBGet(rocksdb::DB*, rocksdb::Transaction*, rocksdb::ReadOptions&, uint16_t, uint64_t, bool, uint64_t*, std::__cxx11::string*, bool*)’:
util/transaction_test_util.cc:75:8: error: ‘snprintf’ output may be truncated before the last format character [-Werror=format-truncation=]
Status RandomTransactionInserter::DBGet(
^~~~~~~~~~~~~~~~~~~~~~~~~
util/transaction_test_util.cc:84:11: note: ‘snprintf’ output between 5 and 6 bytes into a destination of size 5
snprintf(prefix_buf, sizeof(prefix_buf), "%.4u", set_i + 1);
~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
util/transaction_test_util.cc: In static member function ‘static rocksdb::Status rocksdb::RandomTransactionInserter::Verify(rocksdb::DB*, uint16_t, uint64_t, bool, rocksdb::Random64*)’:
util/transaction_test_util.cc:245:8: error: ‘snprintf’ output may be truncated before the last format character [-Werror=format-truncation=]
Status RandomTransactionInserter::Verify(DB* db, uint16_t num_sets,
^~~~~~~~~~~~~~~~~~~~~~~~~
util/transaction_test_util.cc:268:13: note: ‘snprintf’ output between 5 and 6 bytes into a destination of size 5
snprintf(prefix_buf, sizeof(prefix_buf), "%.4u", set_i + 1);
~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Closes https://github.com/facebook/rocksdb/pull/3295
Differential Revision: D6609411
Pulled By: maysamyabandeh
fbshipit-source-id: 33f0add471056eb59db2f8bd4366e6dfbb1a187d
Islam AbdelRahman