Summary:
Add --benchmarks=updaterandom for read-modify-write workloads. This is different
from --benchmarks=readrandomwriterandom in a few ways. First, an "operation" is the
combined time to do the read & write rather than treating them as two ops. Second,
the same key is used for the read & write.
Change RandomGenerator to support rows larger than 1M. That was using "assert"
to fail and assert is compiled-away when -DNDEBUG is used.
Add more options to db_bench
--duration - sets the number of seconds for tests to run. When not set the
operation count continues to be the limit. This is used by random operation
tests.
--use_snapshot - when set GetSnapshot() is called prior to each random read.
This is to measure the overhead from using snapshots.
--get_approx - when set GetApproximateSizes() is called prior to each random
read. This is to measure the overhead for a query optimizer.
Task ID: #
Blame Rev:
Test Plan:
run db_bench
Revert Plan:
Database Impact:
Memcache Impact:
Other Notes:
EImportant:
- begin *PUBLIC* platform impact section -
Bugzilla: #
- end platform impact -
Reviewers: dhruba
Reviewed By: dhruba
Differential Revision: https://reviews.facebook.net/D9267
Summary: Updated TOOL_CHAIN_LIB_BASE to use the third-party version for jemalloc-3.3.1 which contains a bug fix in quarantine.cc. This was detected while debugging valgrind issues with the rocksdb table_test
Test Plan: make table_test;valgrind --leak-check=full ./table_test
Reviewers: dhruba, sheki, vamsi
Reviewed By: sheki
Differential Revision: https://reviews.facebook.net/D9387
Summary:
Ftruncate does not throw an error on disk-full. This causes Sig-bus in
the case where the database tries to issue a Put call on a full-disk.
Use posix_fallocate for allocation instead of truncate.
Add a check to use MMaped files only on ext4, xfs and tempfs, as
posix_fallocate is very slow on ext3 and older.
Test Plan: make all check
Reviewers: dhruba, chip
Reviewed By: dhruba
CC: adsharma, leveldb
Differential Revision: https://reviews.facebook.net/D9291
Summary: ldb_test.py did a lot of assertFalse checks and displayed all the failed messages on the std output making it confusing to tell a successful from a failed run. Also many empty lines used to be needlessly printed. Also added some progression-"feel-good" lines in the tests
Test Plan: python ldb_test.py
Reviewers: dhruba, sheki, dilipj, chip
Reviewed By: dilipj
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9297
Summary: Fix for memory leaks in rocksdb tests. Also modified the variable NUM_FAILED_TESTS to print the actual number of failed tests.
Test Plan: make <test>; valgrind --leak-check=full ./<test>
Reviewers: sheki, dhruba
Reviewed By: sheki
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9333
Summary:
SizeBeingCompacted was called without any lock protection. This causes
crashes, especially when running db_bench with value_size=128K.
The fix is to compute SizeUnderCompaction while holding the mutex and
passing in these values into the call to Finalize.
(gdb) where
#4 leveldb::VersionSet::SizeBeingCompacted (this=this@entry=0x7f0b490931c0, level=level@entry=4) at db/version_set.cc:1827
#5 0x000000000043a3c8 in leveldb::VersionSet::Finalize (this=this@entry=0x7f0b490931c0, v=v@entry=0x7f0b3b86b480) at db/version_set.cc:1420
#6 0x00000000004418d1 in leveldb::VersionSet::LogAndApply (this=0x7f0b490931c0, edit=0x7f0b3dc8c200, mu=0x7f0b490835b0, new_descriptor_log=<optimized out>) at db/version_set.cc:1016
#7 0x00000000004222b2 in leveldb::DBImpl::InstallCompactionResults (this=this@entry=0x7f0b49083400, compact=compact@entry=0x7f0b2b8330f0) at db/db_impl.cc:1473
#8 0x0000000000426027 in leveldb::DBImpl::DoCompactionWork (this=this@entry=0x7f0b49083400, compact=compact@entry=0x7f0b2b8330f0) at db/db_impl.cc:1757
#9 0x0000000000426690 in leveldb::DBImpl::BackgroundCompaction (this=this@entry=0x7f0b49083400, madeProgress=madeProgress@entry=0x7f0b41bf2d1e, deletion_state=...) at db/db_impl.cc:1268
#10 0x0000000000428f42 in leveldb::DBImpl::BackgroundCall (this=0x7f0b49083400) at db/db_impl.cc:1170
#11 0x000000000045348e in BGThread (this=0x7f0b49023100) at util/env_posix.cc:941
#12 leveldb::(anonymous namespace)::PosixEnv::BGThreadWrapper (arg=0x7f0b49023100) at util/env_posix.cc:874
#13 0x00007f0b4a7cf10d in start_thread (arg=0x7f0b41bf3700) at pthread_create.c:301
#14 0x00007f0b49b4b11d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115
Test Plan:
make check
I am running db_bench with a value size of 128K to see if the segfault is fixed.
Reviewers: MarkCallaghan, sheki, emayanke
Reviewed By: sheki
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9279
Summary:
This is a regression caused by
772f75b3fb
If you do "strings libleveldb.a | grep leveldb_build_git_datetime" it will
show you the time when the binary was built.
Test Plan: make check
Reviewers: emayanke
Reviewed By: emayanke
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9273
Summary:
Also added some comments and fixed some bugs in
stats reporting. Now the stats seem to match what is expected.
Test Plan:
[nponnekanti@dev902 /data/users/nponnekanti/rocksdb] ./db_stress --test_batches_snapshots=1 --ops_per_thread=1000 --threads=1 --max_key=320
LevelDB version : 1.5
Number of threads : 1
Ops per thread : 1000
Read percentage : 10
Delete percentage : 30
Max key : 320
Ratio #ops/#keys : 3
Num times DB reopens: 10
Batches/snapshots : 1
Num keys per lock : 4
Compression : snappy
------------------------------------------------
No lock creation because test_batches_snapshots set
2013/03/04-15:58:56 Starting database operations
2013/03/04-15:58:56 Reopening database for the 1th time
2013/03/04-15:58:56 Reopening database for the 2th time
2013/03/04-15:58:56 Reopening database for the 3th time
2013/03/04-15:58:56 Reopening database for the 4th time
Created bg thread 0x7f4542bff700
2013/03/04-15:58:56 Reopening database for the 5th time
2013/03/04-15:58:56 Reopening database for the 6th time
2013/03/04-15:58:56 Reopening database for the 7th time
2013/03/04-15:58:57 Reopening database for the 8th time
2013/03/04-15:58:57 Reopening database for the 9th time
2013/03/04-15:58:57 Reopening database for the 10th time
2013/03/04-15:58:57 Reopening database for the 11th time
2013/03/04-15:58:57 Limited verification already done during gets
Stress Test : 1811.551 micros/op 552 ops/sec
: Wrote 0.10 MB (0.05 MB/sec) (598% of 1011 ops)
: Wrote 6050 times
: Deleted 3050 times
: 500/900 gets found the key
: Got errors 0 times
[nponnekanti@dev902 /data/users/nponnekanti/rocksdb] ./db_stress --ops_per_thread=1000 --threads=1 --max_key=320
LevelDB version : 1.5
Number of threads : 1
Ops per thread : 1000
Read percentage : 10
Delete percentage : 30
Max key : 320
Ratio #ops/#keys : 3
Num times DB reopens: 10
Batches/snapshots : 0
Num keys per lock : 4
Compression : snappy
------------------------------------------------
Creating 80 locks
2013/03/04-15:58:17 Starting database operations
2013/03/04-15:58:17 Reopening database for the 1th time
2013/03/04-15:58:17 Reopening database for the 2th time
2013/03/04-15:58:17 Reopening database for the 3th time
2013/03/04-15:58:17 Reopening database for the 4th time
Created bg thread 0x7fc0f5bff700
2013/03/04-15:58:17 Reopening database for the 5th time
2013/03/04-15:58:17 Reopening database for the 6th time
2013/03/04-15:58:18 Reopening database for the 7th time
2013/03/04-15:58:18 Reopening database for the 8th time
2013/03/04-15:58:18 Reopening database for the 9th time
2013/03/04-15:58:18 Reopening database for the 10th time
2013/03/04-15:58:18 Reopening database for the 11th time
2013/03/04-15:58:18 Starting verification
Stress Test : 1836.258 micros/op 544 ops/sec
: Wrote 0.01 MB (0.01 MB/sec) (59% of 1011 ops)
: Wrote 605 times
: Deleted 305 times
: 50/90 gets found the key
: Got errors 0 times
2013/03/04-15:58:18 Verification successful
Revert Plan: OK
Task ID: #
Reviewers: emayanke, dhruba
Reviewed By: emayanke
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9081
Summary: This option is needed for compilation and the open-sourced rocksdb version wiull need to get it from Makefile
Test Plan: make clean;make
Reviewers: MarkCallaghan, dhruba, sheki, chip
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9243
Summary:
the valgrind version being used is in facebook specific path and should be moved to the fbcode.gcc471.sh file instead of the makefile.
The execution takes the environment's default valgrind version if the fbcode.gcc471.sh's valgrind_version is not available.
Test Plan: make valgrind_check
Reviewers: dhruba, sheki, akushner
Reviewed By: dhruba
Differential Revision: https://reviews.facebook.net/D9213
Summary:
1. Create only 2 levels so that manual compactions are fast.
2. Set target file size to a large value
Test Plan: make clean check
Reviewers: kailiu, zshao
Reviewed By: zshao
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9231
Summary: In light of the new option introduced by commit 806e264350 where the database has an option to compact before flushing to disk, we want the stress test to test both sides of the option. Have made it to 'deterministically' and configurably change that option for reopens.
Test Plan: make db_stress; ./db_stress with some differnet options
Reviewers: dhruba, vamsi
Reviewed By: dhruba
CC: leveldb, sheki
Differential Revision: https://reviews.facebook.net/D9165
Summary:
If there is an error while writing an edit to the manifest file, the manifest
file is closed and reopened to check if the edit made it in. However, if the
re-opening of the manifest is unsuccessful and options.paranoid_checks is set
t true, then the db refuses to accept new puts, effectively putting the db
in readonly mode.
In a future diff, I would like to make the default value of paranoid_check
to true.
Test Plan: make check
Reviewers: sheki
Reviewed By: sheki
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9201
Summary:
valgrind 3.7.0 used currently has a bug that needs LD_PRELOAD being set as a workaround. This caused problems when run on jenkins. 3.8.1 has fixed this issue and we should use it from third party
Also, have done away with log files. The whole output will be there on the terminal and the failed tests will be listed at the end. This is done because jenkins only lets us download the different files and not view them in the browser which is undesirable.
Test Plan: make valgrind_check
Reviewers: akushner, dhruba, vamsi, sheki, heyongqiang
Reviewed By: sheki
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9171
Summary:
Store the last flushed, seq no. in db_impl. Check against it in
transaction Log iterator. Do not attempt to read ahead if we do not know
if the data is flushed completely.
Does not work if flush is disabled. Any ideas on fixing that?
* Minor change, iter->Next is called the first time automatically for
* the first time.
Test Plan:
existing test pass.
More ideas on testing this?
Planning to run some stress test.
Reviewers: dhruba, heyongqiang
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9087
Summary:
The compaction process zeros out sequence numbers if the output is
part of the bottommost level.
The Slice is supposed to refer to an immutable data buffer. The
merger that implements the priority queue while reading kvs as
the input of a compaction run reies on this fact. The bug was that
were updating the sequence number of a record in-place and that was
causing suceeding invocations of the merger to return kvs in
arbitrary order of sequence numbers.
The fix is to copy the key to a local memory buffer before setting
its seqno to 0.
Test Plan:
Set Options.purge_redundant_kvs_while_flush = false and then run
db_stress --ops_per_thread=1000 --max_key=320
Reviewers: emayanke, sheki
Reviewed By: emayanke
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9147
Summary:
When we use -O3, the gcc 4.7.1 compiler generates 'pinsrd' which is
not supported on machines with "vendor_id : AuthenticAMD".
Previous release of rocksdb used -O2.
Optimization -O2 was introduced at
772f75b3fb
Test Plan: make check
Reviewers: chip, heyongqiang, sheki
Reviewed By: sheki
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9093
Summary:
Add a shortcut function to make it easier for people
to efficiently bulk_load data into RocksDB.
Test Plan:
Tried ldb with "--bulk_load" and "--bulk_load --compact" and verified the outcome.
Needs to consult the team on how to test this automatically.
Reviewers: sheki, dhruba, emayanke, heyongqiang
Reviewed By: dhruba
CC: leveldb
Differential Revision: https://reviews.facebook.net/D8907
Summary:
TableCache->file is not used. remove it.
I kept the TableAndFile structure and will clean it up in a future patch.
Test Plan: make clean check
Reviewers: sheki, chip
Reviewed By: chip
CC: leveldb
Differential Revision: https://reviews.facebook.net/D9075
Summary:
This adds the rate_delay_limit_milliseconds option to make the delay
configurable in MakeRoomForWrite when the max compaction score is too high.
This delay is called the Ln slowdown. This change also counts the Ln slowdown
per level to make it possible to see where the stalls occur.
From IO-bound performance testing, the Level N stalls occur:
* with compression -> at the largest uncompressed level. This makes sense
because compaction for compressed levels is much
slower. When Lx is uncompressed and Lx+1 is compressed
then files pile up at Lx because the (Lx,Lx+1)->Lx+1
compaction process is the first to be slowed by
compression.
* without compression -> at level 1
Task ID: #1832108
Blame Rev:
Test Plan:
run with real data, added test
Revert Plan:
Database Impact:
Memcache Impact:
Other Notes:
EImportant:
- begin *PUBLIC* platform impact section -
Bugzilla: #
- end platform impact -
Reviewers: dhruba
Reviewed By: dhruba
Differential Revision: https://reviews.facebook.net/D9045
Summary:
Rocks accumulates recent writes and deletes in the in-memory memtable.
When the memtable is full, it writes the contents on the memtable to
a file in L0.
This patch removes redundant records at the time of the flush. If there
are multiple versions of the same key in the memtable, then only the
most recent one is dumped into the output file. The purging of
redundant records occur only if the most recent snapshot is earlier
than the earliest record in the memtable.
Should we switch on this feature by default or should we keep this feature
turned off in the default settings?
Test Plan: Added test case to db_test.cc
Reviewers: sheki, vamsi, emayanke, heyongqiang
Reviewed By: sheki
CC: leveldb
Differential Revision: https://reviews.facebook.net/D8991
Summary:
1. the default value for key size is still 16
2. enable the ability to set the key size via command line --key_size=
Test Plan:
build & run db_banch and pass some value via command line.
verify it works correctly.
Reviewers: sheki
Reviewed By: sheki
CC: leveldb
Differential Revision: https://reviews.facebook.net/D8943
Summary:
The script valgrind_test.sh runs Valgrind for all tests in the makefile
including leak-checks and outputs the logs for every test in a separate file
with the name "valgrind_log_<testname>". It prints the failed tests in the file
"valgrind_failed_tests". All these files are created in the directory
"VALGRIND_LOGS" which can be changed in the Makefile.
Finally it checks the line-count for the file "valgrind_failed_tests"
and returns 0 if no tests failed and 1 otherwise.
Test Plan: ./valgrind_test.sh; Changed the tests to incorporte leaks and verified correctness
Reviewers: dhruba, sheki, MarkCallaghan
Reviewed By: sheki
CC: zshao
Differential Revision: https://reviews.facebook.net/D8877
Summary:
There was an artifical limit of 50K files per database. This is
insifficient if the database is 1 TB in size and each file is 2 MB.
Test Plan: make check
Reviewers: sheki, emayanke
Reviewed By: emayanke
CC: leveldb
Differential Revision: https://reviews.facebook.net/D8919
Summary:
Use only the counter mechanism. Do away with
incNumFileOpens, incNumFileClose, incNumFileErrors
s/NULL/nullptr/g in db/table_cache.cc
Test Plan: make clean check
Reviewers: dhruba, heyongqiang, emayanke
Reviewed By: dhruba
CC: leveldb
Differential Revision: https://reviews.facebook.net/D8841
Summary:
Currently the test tracks all writes in memory and
uses it for verification at the end. This has 4 problems:
(a) It needs mutex for each write to ensure in-memory update
and leveldb update are done atomically. This slows down the
benchmark.
(b) Verification phase at the end is time consuming as well
(c) Does not test batch writes or snapshots
(d) We cannot kill the test and restart multiple times in a
loop because in-memory state will be lost.
I am adding a FLAGS_multi that does MultiGet/MultiPut/MultiDelete
instead of get/put/delete to get/put/delete a group of related
keys with same values atomically. Every get retrieves the group
of keys and checks that their values are same. This does not have
the above problems but the downside is that it does less amount
of validation than the other approach.
Test Plan:
This whole this is a test! Here is a small run. I am doing larger run now.
[nponnekanti@dev902 /data/users/nponnekanti/rocksdb] ./db_stress --ops_per_thread=10000 --multi=1 --ops_per_key=25
LevelDB version : 1.5
Number of threads : 32
Ops per thread : 10000
Read percentage : 10
Delete percentage : 30
Max key : 2147483648
Num times DB reopens: 10
Num keys per lock : 4
Compression : snappy
------------------------------------------------
Creating 536870912 locks
2013/02/20-16:59:32 Starting database operations
Created bg thread 0x7f9ebcfff700
2013/02/20-16:59:37 Reopening database for the 1th time
2013/02/20-16:59:46 Reopening database for the 2th time
2013/02/20-16:59:57 Reopening database for the 3th time
2013/02/20-17:00:11 Reopening database for the 4th time
2013/02/20-17:00:25 Reopening database for the 5th time
2013/02/20-17:00:36 Reopening database for the 6th time
2013/02/20-17:00:47 Reopening database for the 7th time
2013/02/20-17:00:59 Reopening database for the 8th time
2013/02/20-17:01:10 Reopening database for the 9th time
2013/02/20-17:01:20 Reopening database for the 10th time
2013/02/20-17:01:31 Reopening database for the 11th time
2013/02/20-17:01:31 Starting verification
Stress Test : 109.125 micros/op 22191 ops/sec
: Wrote 0.00 MB (0.23 MB/sec) (59% of 32 ops)
: Deleted 10 times
2013/02/20-17:01:31 Verification successful
Revert Plan: OK
Task ID: #
Reviewers: dhruba, emayanke
Reviewed By: emayanke
CC: leveldb
Differential Revision: https://reviews.facebook.net/D8733
Summary: just record time consumed in compaction
Test Plan: compile
Reviewers: dhruba
Reviewed By: dhruba
CC: leveldb
Differential Revision: https://reviews.facebook.net/D8781
Summary: Added automated valgrind testing for rocksdb by adding valgrind_check in the Makefile
Test Plan: make clean; make all check
Reviewers: dhruba, sheki, MarkCallaghan, zshao
Reviewed By: sheki
CC: leveldb
Differential Revision: https://reviews.facebook.net/D8787
Summary:
* Counters for bytes read and write.
as a part of this diff, I want to=>
* Measure compaction times. @dhruba can you point which function, should
* I time to get Compaction-times. Was looking at CompactRange.
Test Plan: db_test
Reviewers: dhruba, emayanke
CC: leveldb
Differential Revision: https://reviews.facebook.net/D8763
Summary:
Fixed a bug in the stress-test where the correct size was not being
passed to GenerateValue. This bug was there since the beginning but assertions
were switched on in our code-base only recently.
Added comments on the top detailing how the stress test works and how to
quicken/slow it down after investigation.
Test Plan: make all check. ./db_stress
Reviewers: dhruba, asad
Reviewed By: dhruba
CC: vamsi, sheki, heyongqiang, zshao
Differential Revision: https://reviews.facebook.net/D8727
Summary: LDB tool to print the deleted/put keys in hex in the wal file.
Test Plan: run ldb on a db to check if output was satisfactory
Reviewers: dhruba
Reviewed By: dhruba
CC: leveldb
Differential Revision: https://reviews.facebook.net/D8691
Summary:
Changed the Get and Scan options with openForReadOnly mode to have access to the memtable.
Changed the visibility of NewInternalIterator in db_impl from private to protected so that
the derived class db_impl_read_only can call that in its NewIterator function for the
scan case. The previous approach which changed the default for flush_on_destroy_ from false to true
caused many problems in the unit tests due to empty sst files that it created. All
unit tests pass now.
Test Plan: make clean; make all check; ldb put and get and scans
Reviewers: dhruba, heyongqiang, sheki
Reviewed By: dhruba
CC: kosievdmerwe, zshao, dilipj, kailiu
Differential Revision: https://reviews.facebook.net/D8697
Summary:
* Introduce is histogram in statistics.h
* stop watch to measure time.
* introduce two timers as a poc.
Replaced NULL with nullptr to fight some lint errors
Should be useful for google.
Test Plan:
ran db_bench and check stats.
make all check
Reviewers: dhruba, heyongqiang
Reviewed By: dhruba
CC: leveldb
Differential Revision: https://reviews.facebook.net/D8637
Summary:
I missed InitTestDb() in one of my tess. InitTestDb() initializes the test directory, without which the test will throw IO error.
This problem didn't occur before because I've already run the tests before so the test directory is already there.
Test Plan:
Reviewers: dhruba
CC:
Task ID: #
Blame Rev:
Summary:
auto_roll_logger_test is failing because it cannot create the test dir, leading to IO error: /tmp/leveldbtest-6108/db_log_test/LOG: No such file or directory.
I'll temporary remove the unit test and will revert the test this problem is solved.
Test Plan:
make all check
Reviewers: dhruba
CC: leveldb
Task ID: #
Blame Rev:
Summary:
c_test: db/filename.cc:74: std::string leveldb::DescriptorFileName(const string&,....
Test Plan:
this is a failure in a unit test
Differential Revision: https://reviews.facebook.net/D8667
Summary:
The sequence numbers in each record eat up plenty of space on storage.
The optimization zeroes out sequence numbers on kvs in the Lmax
layer that are earlier than the earliest snapshot.
Test Plan: Unit test attached.
Differential Revision: https://reviews.facebook.net/D8619
Summary:
flush_on_destroy has a default value of false and the memtable is flushed
in the dbimpl-destructor only when that is set to true. Because we want the memtable to be flushed everytime that
the destructor is called(db is closed) and the cases where we work with the memtable only are very less
it is a good idea to give this a default value of true. Thus the put from ldb
wil have its data flushed to disk in the destructor and the next Get will be able to
read it when opened with OpenForReadOnly. The reason that ldb could read the latest value when
the db was opened in the normal Open mode is that the Get from normal Open first reads
the memtable and directly finds the latest value written there and the Get from OpenForReadOnly
doesn't have access to the memtable (which is correct because all its Put/Modify) are disabled
Test Plan: make all; ldb put and get and scans
Reviewers: dhruba, heyongqiang, sheki
Reviewed By: heyongqiang
CC: kosievdmerwe, zshao, dilipj, kailiu
Differential Revision: https://reviews.facebook.net/D8631
Summary: Fix the warning [-Werror=format-security] and [-Werror=unused-result].
Test Plan:
enforced the Werror and run make
Task ID: 2101673
Blame Rev:
Reviewers: heyongqiang
Differential Revision: https://reviews.facebook.net/D8553
Summary:
$SUBJECT -- cosmetic fix for histograms, print P75/P99, and
make sure zlib is enabled for our command line tools.
Test Plan: compile, test db_bench with --compression_type=zlib
Reviewers: heyongqiang
Reviewed By: heyongqiang
CC: adsharma, leveldb
Differential Revision: https://reviews.facebook.net/D8445
Summary:
* Add a SplitByTTLLogger to enable this feature. In this diff I implemented generalized AutoSplitLoggerBase class to simplify the
development of such classes.
* Refactor the existing AutoSplitLogger and fix several bugs.
Test Plan:
* Added a unit tests for different types of "auto splitable" loggers individually.
* Tested the composited logger which allows the log files to be splitted by both TTL and log size.
Reviewers: heyongqiang, dhruba
Reviewed By: heyongqiang
CC: zshao, leveldb
Differential Revision: https://reviews.facebook.net/D8037
Mark Callaghan