|
| 1 | +The following instructions are only to be applied when performing a code review. |
| 2 | + |
| 3 | +# Copilot Instructions for Apache Pulsar |
| 4 | + |
| 5 | +## Project Context |
| 6 | + |
| 7 | +Apache Pulsar is a distributed messaging and streaming platform designed for high throughput, low latency, and |
| 8 | +horizontal scalability. |
| 9 | + |
| 10 | +The codebase contains performance-critical, asynchronous, and concurrency-sensitive components such as brokers, storage |
| 11 | +clients, and networking layers. |
| 12 | + |
| 13 | +Code reviews should prioritize: |
| 14 | + |
| 15 | +* correctness |
| 16 | +* thread safety |
| 17 | +* performance |
| 18 | +* maintainability |
| 19 | +* backward compatibility |
| 20 | + |
| 21 | +--- |
| 22 | + |
| 23 | +# Java Coding Conventions |
| 24 | + |
| 25 | +Apache Pulsar follows the Sun Java Coding Conventions with additional project-specific rules. |
| 26 | + |
| 27 | +## Formatting |
| 28 | + |
| 29 | +* Use **4 spaces** for indentation. |
| 30 | +* **Tabs must never be used**. |
| 31 | +* Always use **curly braces**, even for single-line `if` statements. |
| 32 | + |
| 33 | +Example: |
| 34 | + |
| 35 | +```java |
| 36 | +if (condition) { |
| 37 | + doSomething(); |
| 38 | +} |
| 39 | +``` |
| 40 | + |
| 41 | +## Javadoc |
| 42 | + |
| 43 | +* Do **not include `@author` tags** in Javadoc. |
| 44 | + |
| 45 | +## TODO Comments |
| 46 | + |
| 47 | +All TODO comments must reference a GitHub issue. |
| 48 | + |
| 49 | +Example: |
| 50 | + |
| 51 | +```java |
| 52 | +// TODO: https://github.com/apache/pulsar/issues/XXXX |
| 53 | +``` |
| 54 | + |
| 55 | +--- |
| 56 | + |
| 57 | +# Dependencies |
| 58 | + |
| 59 | +Prefer existing dependencies instead of introducing new libraries. |
| 60 | + |
| 61 | +The Pulsar codebase commonly uses: |
| 62 | + |
| 63 | +* **Apache Commons libraries or Guava** for utilities |
| 64 | +* **FastUtil** for optimized type specific collections |
| 65 | +* **JCTools** for high performance concurrent data structures |
| 66 | +* **RoaringBitmap** for compressed bitmaps (bitsets) |
| 67 | +* **Caffeine** for caching |
| 68 | +* **Jackson** for JSON handling |
| 69 | +* **Prometheus Java simpleclient** (or newer **Prometheus Java Metrics Library**) for metrics |
| 70 | +* **OpenTelemetry API** for metrics |
| 71 | +* **Netty** for networking and buffers |
| 72 | + |
| 73 | +When introducing a new dependency: |
| 74 | + |
| 75 | +* justify why existing dependencies are insufficient |
| 76 | +* ensure required license files and notices are updated |
| 77 | + |
| 78 | +--- |
| 79 | + |
| 80 | +# Logging Guidelines |
| 81 | + |
| 82 | +* Use **SLF4J** for logging. |
| 83 | +* Do **not use** `System.out` or `System.err`. |
| 84 | +* Assume production commonly runs at **INFO** log level. |
| 85 | +* Avoid excessive logging in hot paths. |
| 86 | +* Guard expensive `DEBUG` and `TRACE` logging with `log.isDebugEnabled()` or `log.isTraceEnabled()`. |
| 87 | +* Avoid logging stack traces at `INFO` and lower levels. |
| 88 | + |
| 89 | +Log messages should be: |
| 90 | + |
| 91 | +* clear and descriptive |
| 92 | +* capitalized |
| 93 | +* understandable without reading the code |
| 94 | + |
| 95 | +--- |
| 96 | + |
| 97 | +# Resource and Memory Management |
| 98 | + |
| 99 | +Ensure resources are always closed correctly. |
| 100 | + |
| 101 | +Prefer: |
| 102 | + |
| 103 | +```java |
| 104 | +try (InputStream in = ...) { |
| 105 | + // use resource |
| 106 | +} |
| 107 | +``` |
| 108 | + |
| 109 | +Avoid leaks for: |
| 110 | + |
| 111 | +* streams |
| 112 | +* network connections |
| 113 | +* executors |
| 114 | +* buffers |
| 115 | + |
| 116 | +For internal networking/messaging paths, prefer **Netty `ByteBuf`** over `ByteBuffer` unless an external API requires |
| 117 | +`ByteBuffer`. |
| 118 | + |
| 119 | +--- |
| 120 | + |
| 121 | +# Configuration Guidelines |
| 122 | + |
| 123 | +When adding configuration options: |
| 124 | + |
| 125 | +* use clear and descriptive names |
| 126 | +* provide sensible default values |
| 127 | +* update default configuration files |
| 128 | +* document the configuration option |
| 129 | + |
| 130 | +--- |
| 131 | + |
| 132 | +# Concurrency Guidelines |
| 133 | + |
| 134 | +Pulsar is designed as a low-latency asynchronous system. |
| 135 | + |
| 136 | +Verify: |
| 137 | + |
| 138 | +* public classes are **thread-safe** |
| 139 | +* shared mutable state is protected |
| 140 | +* mutations occur on the intended thread |
| 141 | +* fine-grained synchronization is preferred |
| 142 | +* threads have meaningful names for diagnostics |
| 143 | + |
| 144 | +If a class is not thread-safe, annotate it with: |
| 145 | + |
| 146 | +```java |
| 147 | +@NotThreadSafe |
| 148 | +``` |
| 149 | + |
| 150 | +Prefer **OrderedExecutor** for ordered asynchronous execution. |
| 151 | + |
| 152 | +--- |
| 153 | + |
| 154 | +# Asynchronous Programming Guidelines |
| 155 | + |
| 156 | +Pulsar relies heavily on `CompletableFuture` and asynchronous pipelines. |
| 157 | + |
| 158 | +Prefer `CompletableFuture` APIs over `ListenableFuture` for new code. |
| 159 | + |
| 160 | +## Avoid Blocking in Async Paths |
| 161 | + |
| 162 | +Do not introduce blocking operations in asynchronous execution paths. |
| 163 | + |
| 164 | +Examples of blocking operations: |
| 165 | + |
| 166 | +* `Thread.sleep` |
| 167 | +* `Future.get()` |
| 168 | +* `CompletableFuture.join()` |
| 169 | +* blocking IO operations |
| 170 | + |
| 171 | +Blocking calls must not run on event loop or async execution threads. |
| 172 | + |
| 173 | +## Avoid Nested Futures |
| 174 | + |
| 175 | +Avoid returning nested futures such as: |
| 176 | + |
| 177 | +```java |
| 178 | +CompletableFuture<CompletableFuture<T>> |
| 179 | +``` |
| 180 | + |
| 181 | +Prefer flattening with `thenCompose`. |
| 182 | + |
| 183 | +## Asynchronous Exception Handling |
| 184 | + |
| 185 | +Methods returning `CompletableFuture` must not throw synchronous exceptions directly. |
| 186 | + |
| 187 | +Incorrect: |
| 188 | + |
| 189 | +```java |
| 190 | +public CompletableFuture<Void> operation() { |
| 191 | + if (error) { |
| 192 | + throw new IllegalStateException("unexpected state"); |
| 193 | + } |
| 194 | + return CompletableFuture.completedFuture(null); |
| 195 | +} |
| 196 | +``` |
| 197 | + |
| 198 | +Use returned futures to propagate failures: |
| 199 | + |
| 200 | +```java |
| 201 | +return CompletableFuture.failedFuture(exception); |
| 202 | +``` |
| 203 | + |
| 204 | +This also applies to argument validation in async methods: |
| 205 | + |
| 206 | +```java |
| 207 | +if (arg == null) { |
| 208 | + return CompletableFuture.failedFuture(new IllegalArgumentException("arg")); |
| 209 | +} |
| 210 | +``` |
| 211 | + |
| 212 | +Throwing exceptions inside async stages such as `thenApply`, `thenCompose`, `thenRun`, `handle`, or `whenComplete` |
| 213 | +is acceptable: |
| 214 | + |
| 215 | +```java |
| 216 | +return future.thenApply(v -> { |
| 217 | + if (error) { |
| 218 | + throw new IllegalStateException("unexpected state"); |
| 219 | + } |
| 220 | + return result; |
| 221 | +}); |
| 222 | +``` |
| 223 | + |
| 224 | +--- |
| 225 | + |
| 226 | +# Backward Compatibility |
| 227 | + |
| 228 | +Apache Pulsar maintains strong compatibility guarantees. |
| 229 | + |
| 230 | +Changes must not break: |
| 231 | + |
| 232 | +* public APIs |
| 233 | +* client compatibility |
| 234 | +* wire protocol compatibility |
| 235 | +* metadata or serialized formats |
| 236 | + |
| 237 | +Servers must be compatible with both older and newer clients. |
| 238 | + |
| 239 | +Flag any change that may break compatibility. |
| 240 | + |
| 241 | +--- |
| 242 | + |
| 243 | +# Testing Guidelines |
| 244 | + |
| 245 | +## Unit testing |
| 246 | + |
| 247 | +* TestNG is used as the testing framework |
| 248 | +* Mocking uses Mockito |
| 249 | +* Assertions should prefer using AssertJ library with descriptions over using TestNG assertions |
| 250 | +* Awaitility should be used to handle assertions with asynchronous conditions together with AssertJ |
| 251 | + |
| 252 | +# Testing Expectations |
| 253 | + |
| 254 | +Every feature or bug fix should include tests. |
| 255 | + |
| 256 | +Verify: |
| 257 | + |
| 258 | +* unit tests exist |
| 259 | +* edge cases are covered |
| 260 | +* failure scenarios are tested |
| 261 | +* tests are deterministic and stable |
| 262 | +* tests avoid `sleep`-based timing assumptions |
| 263 | +* tests include timeouts to prevent hangs |
| 264 | + |
| 265 | +Integration tests may be required for distributed components. |
| 266 | + |
| 267 | +--- |
| 268 | + |
| 269 | +# Pull Request Review Guidance |
| 270 | + |
| 271 | +When reviewing a pull request, Copilot should: |
| 272 | + |
| 273 | +* verify Java coding conventions are followed |
| 274 | +* detect thread safety risks |
| 275 | +* flag blocking operations in async paths |
| 276 | +* detect improper `CompletableFuture` usage |
| 277 | +* detect unnecessary dependencies and missing license updates |
| 278 | +* ensure logging follows project guidelines |
| 279 | +* verify backward compatibility |
| 280 | +* suggest missing tests when appropriate |
| 281 | + |
| 282 | +Focus feedback on correctness, reliability, and maintainability. |
0 commit comments